| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Updated parser code #15006
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
Updated parser code #15006
Conversation
|
✅ Hi, I am the SymPy bot (v130). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.2.1. 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. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
|
@NikhilPappu Please fill out the pull request template so we know what the PR is about. This PR also looks very large, like your last one. It is much easier for us to review if you break it into atomic changes spread across multiple PRs. It will speed up review if you do that. |
|
Looks like you found another bug in the bot. Thanks @NikhilPappu! |
|
Also please write real commit messages. "Updated code" could literally apply to any commit. The commit message should describe what the commit changes in a way that someone who comes across it in a year from now could understand what was changed and, importantly, why. |
|
@NikhilPappu Here is a nice guide to writing good commit messages: https://chris.beams.io/posts/git-commit/ |
|
We also have a guide in our own development workflow https://github.com/sympy/sympy/wiki/Development-workflow#writing-commit-messages. I might take a look at the guide Jason posted and see if our guide can be improved. |
|
Also one thing that I would add that doesn't seem to be in either guide is to never use the |
|
@asmeurer @moorepants |
NikhilPappu
force-pushed
the
autolev_parser
branch
2 times, most recently
from
a91b1b1 to
6c48670
Compare
The major changes in this commit include the code I have changed in _listener_autolev_antlr.py. The changes to other files are minor. I have also made the changes requested in PR sympy#14758 after it had been merged. Some specific changes are: 1. Changed the input rule in the grammar and parser code to fix errors. 2. Added a flag include_pydy in parse_autolev. 3. Changed the doctest in __init__.py to make it look better. 4. Removed the print option. stdout is now the default. 5. Made various changes to _listener_autolev_antlr to parse more files. Revamped the processVariables function quite a bit. Changed the mass function and the pydy output code a bit followed by some minor changes. 6. I have also added a .subs(kindiffdict()) in the forcing full method of kane.py. This is required for the pydy numerical code to work in some cases. This doesn't break any of the test cases. 7. Changed zip to zip_longest in test_autolev.py. Also added commented code for the tests in the GitLab repo.
NikhilPappu
force-pushed
the
autolev_parser
branch
from
851d22d to
384217c
Compare
NikhilPappu
force-pushed
the
autolev_parser
branch
from
227af54 to
7d99bef
Compare
|
@moorepants I have changed the |
|
@NikhilPappu resolve the merge conflicts. |
|
@moorepants I resolved them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments added. I'm going to also test this locally and will give any feedback if needed.
sympy/parsing/autolev/__init__.py
Outdated
| 2. Can be a string containing Autolev code. | ||
| ========= | ||
| autolev_code : str, any object with a readlines() method (such as a file handle or StringIO) | ||
| Autolev code... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dangling sentence, make complete.
sympy/parsing/autolev/__init__.py
Outdated
| INPUT Q1=.1,Q2=.2,U1=0,U2=0 | ||
| INPUT TFINAL=10, INTEGSTP=.01 | ||
| CODE DYNAMICS() some_filename.c | ||
| ------------FILE-------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this render correctly when parsed? Don't you need a :: or .. code: XXX?
sympy/parsing/autolev/__init__.py
Outdated
| >>> l.append("INPUT TFINAL=10, INTEGSTP=.01)" # doctest: +SKIP | ||
| >>> l.append("CODE DYNAMICS() double_pendulum.c)" # doctest: +SKIP | ||
| >>> parse_autolev("\\n".join(l), "print") # doctest: +SKIP | ||
| >>> print(parse_autolev(open("double_pendulum.al"), include_numeric=True)) # doctest: +SKIP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use:
with open("double_pendulum.al", 'r') as f:
parse_autolev(f, include_numeric=True)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't skip the doctest if possible.
Load the text like:
>>> my_al_text = """\
stuff
more stuff
another thing"""There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the doctest and the test passes locally but Travis won't pass it because of some antlr import stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does antlr need to be added to the .travis.yml file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It already exists. I think there should be a way to skip the doctest if it doesn't find antlr. The unit tests do this. The latex parser also uses doctest skip. I am assuming this is because it doesn't work otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because antlr is not installed in the doc tests section on travis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open an issue for this "antlr not available for doctests".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the doctests_depends_on decorator to skip the doctest if a module isn't installed.
sympy/parsing/autolev/__init__.py
Outdated
| import sympy.physics.mechanics as me | ||
| import sympy as sm | ||
| import math as m | ||
| import numpy as np | ||
| <BLANKLINE> | ||
|
|
||
| q1, q2, u1, u2 = me.dynamicsymbols('q1 q2 u1 u2') | ||
| q1d, q2d, u1d, u2d = me.dynamicsymbols('q1 q2 u1 u2', 1) | ||
| l, m, g=sm.symbols('l m g', real=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space around equals, please run PEP8 linter on all code
| # Works | ||
| # Numerical parts for such kind of things won't work. | ||
| # The current numerical parts only work with the simple kane's method use (no_args), | ||
| # no config constraints etc. Have to add auxiliary stuff later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An issue should be opened about supporting config constraints, auxiliary speeds, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
First error I got when trying locally is: I then tried: There does not seem to be any package called |
|
I take that back. It is just conda's poor search command: |
|
New error. I tried this file https://github.com/moorepants/dissertation/blob/master/src/eom/Whipple.al, which is a reasonably complex Autolev file and got a KeyError. There is no custom error message, so no real idea what causes this. |
|
@moorepants I'll make the changes shortly. The whipple.al file should work (other than the linearization part) if the errors are fixed. I'll look into this when I find time. |
sympy/physics/mechanics/kane.py
Outdated
| @@ -638,7 +638,7 @@ def forcing_full(self): | |||
| if not self._fr or not self._frstar: | |||
| raise ValueError('Need to compute Fr, Fr* first.') | |||
| f1 = self._k_ku * Matrix(self.u) + self._f_k | |||
| return -Matrix([f1, self._f_d, self._f_dnh]) | |||
| return -Matrix([f1.subs(self.kindiffdict()), self._f_d.subs(self.kindiffdict()), self._f_dnh.subs(self.kindiffdict())]) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think xreplace() should, in general, be used instead of subs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs a unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure that this is right place to do this. It could have quite the affect on speed of computation.
This is the kind of thing that needs to be in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used xreplace. There doesn't seem to be any difference in the runtime of the current mechanics tests though. I think we could let it stay here.
As for a unit test, I am not sure how to construct one. Even in the examples I have come across, there doesn't seem to be a difference in the kanes equations but PyDy's System class is unable to do the integration when this isn't present but is doing fine after this change.
You can try running tutor6.py available in the GitLab repo with and without this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets exacerbated on larger problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about a workaround. I shall move this to another PR then.
sympy/parsing/tests/test_autolev.py
Outdated
| # for i in l: | ||
| # in_filepath = os.path.join("autolev-tutorial", i + ".al") | ||
| # out_filepath = os.path.join("autolev-tutorial", i + ".py") | ||
| # _test_examples(in_filepath, out_filepath, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't leave commented code in like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left these like this because the autolev-tutorial and dynamics-online directories can be copied over from the GitLab repo and tested by uncommenting this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It would be better to have them uncommented and use an if statement that runs them if the files are there, but doesn't if the files aren't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds better. I shall change it.
moorepants
added
PR: author's turn
Also fixed the position vector error observed in sympy#15165.
|
@moorepants I have addressed the comments and made some changes. I have also fixed the position vector error observed in #15165. I think this PR should be almost ready for merging now. As for whipple.al, I will have to run it line by line and check the reasons for the errors. I shall do this later. |
sympy/parsing/autolev/__init__.py
Outdated
| ... INPUT M=1,G=9.81,L=1\\n\\ | ||
| ... INPUT Q1=.1,Q2=.2,U1=0,U2=0\\n\\ | ||
| ... INPUT TFINAL=10, INTEGSTP=.01\\n\\ | ||
| ... CODE DYNAMICS() some_filename.c" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the \\n\\ the only way to get this work? Makes ti quite ugly...
You can do this too:
>>> my_text = ('first line',
... 'second line',
... 'third line')
>>> my_text = '\n'.join(my_text)Maybe that is a little cleaner??
|
Looks like you need something that says "if using symengine" use subs, otherwise use xreplace.: |
|
Opened issue here: symengine/symengine#1456 |
|
I'm fine with merging. Let me know when tests pass and last things are done. |
NikhilPappu
force-pushed
the
autolev_parser
branch
from
271fd6b to
6405521
Compare
NikhilPappu
force-pushed
the
autolev_parser
branch
from
6405521 to
73333b5
Compare
|
@moorepants I think this can be merged now. |
|
Great! |
The major changes in this commit include the code I have changed
in _listener_autolev_antlr.py. The changes to other files are
minor. I have also made the changes requested in PR #14758
after it had been merged.
Some specific changes are:
__init__.pyto make it look better.more files. Revamped the processVariables function quite a bit.
Changed the mass function and the pydy output code a bit followed
by some minor changes.
of kane.py. This is required for the pydy numerical code to work in
some cases. This doesn't break any tests.
parsing
quite a bit. Changed the mass function and the pydy output code a bit followed
by some minor changes. Made some minor changes to other files in parsing.autolev.
tests and changed zip to zip_longest which is more correct.
physics.mechanics
numerical code to work in some cases. This doesn't break any tests.