Bug/fix inappropriate symbol and ! for factorial

[m-messer]

Removed -∞ == -inf test when strict_syntax is enabled. Test was expecting a parsing failure, but Sympy parses ∞.

Additionally, implemented ! and !! to be parsed to sympy when using elementary_functions

[peterbjohnson]

Please can we discuss this (with Phil). I wonder if it's better to build a transformer at the token level and register it with the other transformers, rather than running a custom loop on the string.

Possible issues currently:

• triple factorial (!!!) and so on up to n
• precedence e.g.: 2^3!
• functions e.g. sin(1)!! becomes sinfactorial2((1)) which I think at best would give sin*factorial2((1)) which is incorrect?

We should be using the tokeniser that we have, which already manages parentheses and precedence. That should make a more robust and extensible post-fix operator, similar to implicit multiplicaiton etc.

[m-messer]

Thank you for the feedback. I'll add it to the list to discuss in our next meeting, which I will book soon.

This was one failing test, which I thought would be an easy fix! The existing parsing of elementary functions works by replacing key terms with defaults. For example, SIN would become sin. This process does not work for !, as the transformation is more complex; n! has to be transformed into factorial(n), not n factorial.

This might be something we do in Wolfram instead of Python?

1. n!!! does fail, thought only because it is missing a trailing bracket
2. 2^3! returns a response of 2^{6}, which also indicates that factorial is being evaluated. I don't think this is what we want, so as you say, it might not be the correct solution
3. sin(x)!! does parse as you suggested, but then fails to parse, if implicit multiplication is not enabled.

Overall, I think you are right that this is not the correct solution. Looking through Karl's code this morning, he had a warning message for ! to use factorial() instead.

With a quick test on the Wolfram evaluation function, ! is supported natively.

My recommendation would be that, as we develop Wolfram, we include the factorial function into those evaluation functions, remove this work, and delete the failing test.

[peterbjohnson]

Thanks for revisiting this.

I agree to splitting the infinity test and the factorial part.

For the factorial we don't necessarily need to give up completely.

Did you try using the transformer that ships with SymPy?
https://docs.sympy.org/latest/modules/parsing.html#sympy.parsing.sympy_parser.factorial_notation

https://github.com/sympy/sympy/blob/16fa855354eb7bcabd3fe10993841e03b1382692/sympy%2Fparsing%2Fsympy_parser.py#L624

I've attached a MWE.
factorial_MWE.py
It only works for single factorial (factorial2() exists, but there's no transformer to go with it).