Bug/gh 223 implicit multi custom symbol

[m-messer]

UPDATED

Problem:
Support for custom multi-character symbols, such as bc, is not correctly supported by compareExpressions when using implicit multiplication.

For example:
Answer: $a/(bc*d)$
Symbols: a, bc, d

Valid solutions that are marked as incorrect
$a/(bcd)$
$a/bcd$

Identified bug:
The parser was unable to identify the multicharacter symbols in the expression.
For example, the symbol bc in the answer bc*d and the submission bcd will be parsed as b * c * d, as the parser works on identifying symbols through space characters. Therefore, it could not find the symbol bc.

Solution:
utlitiy/expression_utlities.py line 742 - ensure that a space is on either side of any symbols that are parsed as parameters.
Sympy will then strip any spaces when parsing the symbols.

Further changes:

• docs/user.md - Removed reference to the workaround which is no longer needed
• evaluation_tests.py - Added tests for validating if multicharacters and implicit multiplication works
• preview_tests.py - Added a multicharacter implicit multiplication test for preview
• tests/multi_character_tests.py - Added a further in-depth test suite
• tests/symbolic_evaluation_tests.py - Unrelated fix for failing tests for reserved special functions Lambda and chi, which were added in PR Feature/unicode support #237
• __init__.py - Made utility a Python package
• utlity/expression_utilities.py - Mostly formatting changes other than the fix mentioned above

[peterbjohnson]

Similar to my other review, we need to review whether this approach is appropriate, or whether we should be integrating more with the existing parsing.

Regarding adding a parameter that only works with another parameter, that may be problematic. E.g. (for illustration)implicit_multiplication_higher_precedence_A and implicit_multiplication_higher_precedence_B where e.g. A is normalise and B is nonnormalised - or whatever we agree on.

I'm also not clear what 'normalise' means here. It has many meanings generally and in this context it's not obvious to me.

[m-messer]

I took a look at the transformers, including the depths of Sympy and its tokeniser. Sympy doesn't handle the case where bc is registered as a symbol with sympy, and an input of a/(bcd) is provided, as the tokeniser will treat that bcd as a token which does not match an existing symbol. Sympy does not support substring tokenisation, which is why I chose this approach.

If we are happy to go ahead with this, I will rename them implicit_multiplication_higher_precedence and implicit_multiplication_higher_precedence_multi_character. I had it has two separate parameters, as I wasn't sure if we required it anywhere else.

[peterbjohnson]

I don't know if it's needed in other cases, I was just following your comment that it's only needed with implicit multiplication.

I think implicit multiplication works ok with bcd as bc•d where bc is a multi letter symbol? If so, I'd like to understand why it doesn't work with higher precedence implicit multiplication, and how the suggested solution works despite that.

Easiest will be for me to ask you when I have a chance.

[peterbjohnson]

The comment is redundant (and is inconsistent because it still contains an 'e'.

[peterbjohnson]

Most of these comments are redundant

[peterbjohnson]

All good.

I've made a couple of inline comments that need addressing.

Also for the class that tests 'implicit' multiplication, I can see from the code that it all uses the convention of higher precedence for implicit multiplication. The naming of all of the functions just refers to 'implicit' though, which is not a complete description and can be confusing. Maybe you already thought about this and decided that it's fine. But if you haven't thought about it, consider making it clear/explicit that those tests are for the higher precedence convention. E.g. a/bcd = a/(bc.d) is only true for that convention, it's not true otherwise.