Skip to content

Conversation

prajwalgatti
Copy link

@prajwalgatti prajwalgatti commented Nov 18, 2017

Fixes #13612

Now the code produces the following output:
>>>decompogen(Max(x, 3), x)
[Max]
>>>decompogen(Max(Min(sin(tan(x**4)), pi), sin(tan(x))), x)
[Max, sin(x), tan(x), Min, sin(x), tan(x), x**4]

@prajwalgatti
Copy link
Author

@smichr, please review

@smichr
Copy link
Member

smichr commented Nov 19, 2017

ping @aktech , can you take a look at this? It seems too fragile to only be traversing certain args. Perhaps you could make a better suggestion.

@@ -47,6 +50,14 @@ def decompogen(f, symbol):
result += [f.subs(f.args[0], symbol)] + decompogen(f.args[0], symbol)
return result

# ===== Max/Min Functions ===== #
if isinstance(f, (Max, Min)):
result += [type(f)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding what you are going to traverse in the for loop makes this fragile -- trust recursion here:

if symbol not in f.free_symbols or f == symbol:
    return [f]

if isinstance(f, (Function, Pow)):
...
if isinstance(f, (Max, Min)):
    rv = [type(f)]
    for i in f.args:
        rv.extend(decompogen(i, symbol))
    return rv

But I don't know what is going to be done with the results and whether it is important to know that Min and Max were present...so maybe it should start out as rv = [].

@smichr smichr added PR: author's turn The PR has been reviewed and the author needs to submit more changes. and removed PR: sympy's turn labels Jan 24, 2018
@czgdp1807
Copy link
Member

@prajwalgatti Are you still interested to work on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Please take over PR: author's turn The PR has been reviewed and the author needs to submit more changes. solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

decompogen doesn't like Min/Max
5 participants