Skip to content

Issue 2328 Solved #1106

Closed
wants to merge 2 commits into from

4 participants

@ghost
ghost commented Mar 6, 2012

TODO respond to feedback

http://code.google.com/p/sympy/issues/detail?id=2328&q=label%3AEasyToFix&colspec=ID%20Type%20Status%20Priority%20Milestone%20Owner%20Summary%20Stars

The solution to the above issue. Issue number 2328.
I'm checking the hints dictionary to see if it contains valid keys, if it does not contain valid keys i'm raising a Type Error.

@scolobb scolobb and 2 others commented on an outdated diff Mar 6, 2012
sympy/core/expr.py
@@ -2509,6 +2509,11 @@ def expand(self, deep=True, modulus=None, power_base=True, power_exp=True, \
See the docstring in function.expand for more information.
"""
+ list_hints = ['power_base','power_exp','force','commutator','tensorproduct','mul','log','multinomial','basic','complex', 'func', 'trig', 'frac', 'numer','denom']
+ if hints:
+ for i in hints.keys():
+ if i not in list_hints:
+ raise TypeError("The arguement passed to the function is not a valid hint \""+i+"\"."
@scolobb
scolobb added a note Mar 6, 2012

arguement -> argument

You may consider using a formulation like "'i' is not a valid hint."; I think that talking about arguments passed to a function makes the error message a bit too long.

@asmeurer
SymPy member
asmeurer added a note Mar 6, 2012

Yes, just do "%d is not a valid hint to expand()." % i.

@smichr
SymPy member
smichr added a note Mar 7, 2012

How about rename list_hints to valid_hints and write it alphabetically as

valid_hints = [
    'base',
    'comnutator',
    ...
    ]

And then keep your checking loop clean:

for i in hints:
    if i not in valid_hints:
        raise ValueError(...) # ValueError, not TypeError, isn't it?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ghost
ghost commented Mar 6, 2012

Awesome :) I will do that :) Any other things i can improve upon. I'm new to this and i'm ready to learn from my mistakes :)

@asmeurer
SymPy member
asmeurer commented Mar 6, 2012

Where did you get this list of possible hints?

@ghost
ghost commented Mar 7, 2012

I added all the hints in the expand function and ran the tests. A few other functions were calling expand with different hints as well(like 2 or 3 different hints). So i added those hints as well :)

@asmeurer
SymPy member

I think maybe the solution from #1117 is better.

@asmeurer
SymPy member

That is, assuming it works. I didn't test it.

@smichr
SymPy member
smichr commented Apr 14, 2012

@vinoo , are you still willing to work on this? If so, I think you should restructure this so you have a list of options (which are hints for hints) that you check if a hint is not recognized. There is no need to do the checking up-front since it will be done in the loop that loops over the hints.

hint_hints = [
    'commutator',
    'denom',
    'force',
    'frac',
    'numer',
    'tensorproduct',
    ]

...

func = getattr(expr, '_eval_expand_'+hint, None)
if func is not None:
    expr = func(deep=deep, **hints)
elif hint not in hint_hints:
    raise ValueError('Unrecognized hint/option: %s' % hint)
@asmeurer
SymPy member

This is no longer relevant with the new expand API.

@asmeurer asmeurer closed this Aug 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.