Fixing the code generator

[Upabjojr]

The code generator enforces the reading of all variables whenever a constraint has been encountered. In this PR, the constraint is check only if all variables have been read.

Furthermore, indentation has been changes to four spaces and a related bug forcing to always use bugs has been fixed as well.

[Upabjojr]

As mentioned in https://github.com/HPAC/matchpy/issues/41#issuecomment-443281112 , the defect is in the code generation.

[Upabjojr]

I will test in the next days this edit on the Rubi tests.

[coveralls]

Coverage increased (+0.07%) to 96.574% when pulling 8cdfa44b2b4756528d5114b1b69cc8aecbdff395 on Upabjojr:codegen_skip_conditions into 430cc5894c1351e31184907c3c573f4a8e9f1c5e on HPAC:master.

[hbarthels]

Thank you very much!

I will test in the next days this edit on the Rubi tests.

Yes, please keep us updated.

[Upabjojr]

A lot of rubi integration rules are now working in the generated code. Not all are passing, but it's not very clear now whether it is a problem in SymPy or in the code generation of MatchPy.

Do you require tests in MatchPy for this PR @henrik227 ?

[hbarthels]

A lot of rubi integration rules are now working in the generated code. Not all are passing, but it's not very clear now whether it is a problem in SymPy or in the code generation of MatchPy.

Do you require tests in MatchPy for this PR @henrik227 ?

I was about to suggest comparing the behavior of the generated code and the original matcher (the Python object), but then I realized that we already have a test which seems to be doing just that. Since the tests pass, I assume that we don't have a case that triggers this bug. I think it would be good to have a test case for this bug. Do you think that would be a lot of work?

[Upabjojr]

I don't really know right now. There is a simple one with only three rubi rules, maybe it can be adopted to this case.

[hbarthels]

I looked at the discussion in https://github.com/sympy/sympy/pull/14988 to figure out what a minimal set of patterns to trigger this bug would have to look like. I came up with f(x, a, y) and f(x, b, y) where one of the patterns has a constraint that depends on both x and y. There will be two branches because of a and b, and because of y, not all variables will be read before the branching. Do you agree that this should trigger the bug?

There are already long list of patterns for testing in MatchPy, and they are also used for the code generation. I think we just needed to add those pattern (plus matches) to that list. Perhaps it's a good idea to use different names for the symbols, so there is no overlap with the existing patterns.

[Upabjojr]

I looked at the discussion in sympy/sympy#14988 to figure out what a minimal set of patterns to trigger this bug would have to look like. I came up with f(x, a, y) and f(x, b, y) where one of the patterns has a constraint that depends on both x and y. There will be two branches because of a and b, and because of y, not all variables will be read before the branching. Do you agree that this should trigger the bug?

So I gave it a try with this script:

from matchpy import *
from matchpy.matching.code_generation import *

f = Operation.new('f', Arity.variadic)
a = Symbol('a')
b = Symbol('b')
x_ = make_dot_variable('x')
y_ = make_dot_variable('y')

constraint1 = CustomConstraint(lambda x, y: (x > 0) and (y > 0))
constraint2 = CustomConstraint(lambda x, y: (x > 0) or (y > 0))

pattern1 = Pattern(f(x_, a, y_), constraint1)
pattern2 = Pattern(f(x_, b, y_), constraint2)

matcher = ManyToOneMatcher(pattern1, pattern2)

expr1 = f(3, a, 4)
expr2 = f(3, b, 4)

cg = CodeGenerator(matcher)

part1, part2 = cg.generate_code()

def write_output(filename, part1, part2):
    fout = open(filename, "w")
    fout.write(part1)
    fout.write("""
from matchpy import *
f = Operation.new('f', Arity.variadic)
a = Symbol('a')
""")

    fout.write("\n\n")
    fout.write(part2)

Indeed it does trigger the bug on the current master, but it works fine on this PR.

I'm not so sure on where to add this to the test cases. I suppose it should be added to tests/test_matching.py, but it's not very clear how.

[hbarthels]

Thank you very much for fixing this bug!

Since this was a bug with the code generation, we should also test it with the generated code. The way I understand it, the test cases in test_code_generation.py come from the lists in test_matching.py. So if we create another list for the patterns f(x_, a, y_) and f(x_, a, y_) with some subjects, they could be used when testing the code generation. However, I don't know how exactly to do this. @wheerd should know that.

Alternatively, we could use a function similar to the one you added in test_matching.py, but using the code generator like in the script you posted.

[Upabjojr]

I have pushed the updated tests.

Have a look and see if they make sense.

[Upabjojr]

@henrik227 the updated tests fail on the master branch, which is expected.

The problem I had is to fix CustomConstraint into the tests, which was poorly tested.

[wheerd]

@Upabjojr Thank you for the tests :) Catching a TypeError in the test seems a bit hacky though. I think that using a separate test fixture is cleaner. We can add a match_many similar to match_syntactic/ match in the conftest.py.

[Upabjojr]

We can add a match_many similar to match_syntactic/ match in the conftest.py.

Done.

[Upabjojr]

Indeed, MatchPy was missing tests for the ManyToOneMatcher with CustomConstraint and multiple patterns.