Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] code generation for rubi #14988

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@ashishkg0022
Copy link
Contributor

ashishkg0022 commented Jul 28, 2018

For code generation to properly generate files, it needed some change in structure of rubi.
I have generated code only for 3 rules of linear_products. Rest rules are all commented.

In [1]: from sympy.integrals.rubi.generated import *

In [2]: from sympy.abc import x

In [3]: for a, b in match_root(Integral(x,x)):
   ...:     print(a, b)
   ...:     
38 {m ↦ 1, x ↦ x}

In [4]: for a, b in match_root(Integral(x**S(2),x)):
   ...:     print(a, b)
   ...:     

In [5]: 

for line 4 m should be matched to 2.
The code is generated by running python codgen.py

In rubi.py I have made some changes.

@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Jul 28, 2018

Hi, I am the SymPy bot (v120). I'm here to make sure this pull request has a release notes entry. Please read the guide on how to write release notes.

Click here to see the pull request description that was parsed.

For code generation to properly generate files, it needed some change in structure of rubi.
I have generated code only for 3 rules of ` linear_products`. Rest rules are all commented.
```
In [1]: from sympy.integrals.rubi.generated import *

In [2]: from sympy.abc import x

In [3]: for a, b in match_root(Integral(x,x)):
   ...:     print(a, b)
   ...:     
38 {m ↦ 1, x ↦ x}

In [4]: for a, b in match_root(Integral(x**S(2),x)):
   ...:     print(a, b)
   ...:     

In [5]: 
```
for `line 4`  `m` should be matched to `2`.
The code is generated by running `python codgen.py`

In `rubi.py` I have made some changes.

There was an issue with the release notes.

The <!-- BEGIN RELEASE NOTES --> block was not found

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 28, 2018

ping @Upabjojr . I have used code_generation for only 3 rules. So that we can clearly find out why Integral(x**2, x) is not matched, and Integral(x, x) is matched.

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 28, 2018

Also, it is to be noted that without code generation, if we use normal ManyToOneMatcher (rubi) , which has been used to generate the code, it works fine

In [5]: from sympy.integrals.rubi.rubi import *

In [6]: for a, b in rubi.match(Integral(x, x)):
   ...:     print(a, b)
   ...:     
38 {m ↦ 1, x ↦ x}

In [7]: for a, b in rubi.match(Integral(x**S(2), x)):
   ...:     print(a, b)
   ...:     
38 {m ↦ 2, x ↦ x}

In [8]: 
@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 29, 2018

Here is the graph of the 3 rules.
https://drive.google.com/file/d/1s2CewkIWhoUsKrSEE4vly1gOzDudg3Xn/view
quoting @Upabjojr

In the PDF, constraint c0 is checked after state 2212. That should not be the case, as patterns p0 and p1 have not yet been separated and only pattern p1 requires it.

@henrik227

This comment has been minimized.

Copy link

henrik227 commented Jul 29, 2018

Unfortunately, I'm really not familiar with the code generation, and I'm pretty busy right now.

@ashishkg0022 How did you generate this graph? Is this the ManyToOneMatcher graph, or is it based on the generated code?

Some ideas to narrow down what exactly is going wrong:

  • Rename the variables such that the different patterns don't have any variable names in common.
  • Use only one of the two constraints.
  • Could this be some 'off by one' error in the code generation?
@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 30, 2018

How did you generate this graph? Is this the ManyToOneMatcher graph, or is it based on the generated code?

ManyToOneMatcher graph
Generated using .as_graph()

@henrik227

This comment has been minimized.

Copy link

henrik227 commented Jul 30, 2018

I thought the ManyToOneMatcher works correctly, just not the generated code. To me, there seems to be some contradiction. Either the visualization of the graph is wrong, or the placement of c0 is actually correct. In any case, it doesn't say much about the generated code.

@ashishkg0022

This comment has been minimized.

Copy link
Contributor Author

ashishkg0022 commented Jul 30, 2018

yes we too think that there is some issue in ManyToOneMatcher. could it be solved?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 30, 2018

The problem with the generated code is the requirement of variable i3 for condition c0 in transition from state 2212 to 2213. As variable i3 is not read in that transition it cannot be checked in c0, leading to the condition being dropped.

That condition exists in the .check_constraints property, so it's not a graph problem.

I have tried to modify MatchPy in order not to add new conditions to existing transitions, this solved this problem (no c0 in 2212 to 2213) but leads to errors in MatchPy tests, in particular when identical patterns with different conditions exist.

I don't see why there should be condition c0 in that transition, as pattern p0 does not require it and is still not branched off.

My interpretation is that the graph is wrong but for some reason this does not cause errors to the many to one matcher, it only shows in the generated code.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 30, 2018

@ashishkg0022 I think we should leave this PR in this state (i.e. do not modify it), as it is pointing out an issue in MatchPy.

@henrik227

This comment has been minimized.

Copy link

henrik227 commented Nov 19, 2018

It would be useful if we could move the strictly MatchPy-related part of this discussion over to the MatchPy issues. To investigate and solve this problem, it would also be useful to have a minimal code example to reproduce it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.