-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Autolev parser #14758
Autolev parser #14758
Conversation
The directory and files aren't properly in place. |
This should follow some of the same patterns as the LaTeX parser. Files that are autogenerated should contain a warning at the top that they are autogenerated and shouldn't be generated by hand. |
Also it should use the same version of antlr if possible, so that we don't have to install different versions for the tests. |
@asmeurer I will add the warning. I don't think there will be much of a change between different versions of antlr4. I'll update the version to the one used in the LaTeX parser anyway. |
@asmeurer Seems like the antlr version I used (4.7.1) is the same as the one used in the LaTeX parser. |
@asmeurer I don't understand the Travis errors. Could you tell me why they are occurring? Also the line |
@NikhilPappu I would take a look at the LaTeX parser PR (#13706) to get an idea of what sorts of things you should do here to make this work. |
@certik @moorepants Can you please go over this PR and give me feedback? Thanks! |
I think it looks good. Overall I am against checking such large autogenerated files into git, as it increases the size of the git repository (forever). We can generate them for the release tarball. Regarding tests, is this tested on Travis? It should be. I see some test failures, would you mind fixing those please. |
@certik I'm not sure I quite understood what you meant. Do you mean the antlr generated files? How would this be different from the LaTeX parser which also has large autogenerated files? I think generating them only for a release tarball is a good idea though. Did you mean the unit tests? I did add a file test_autolev.py which checks the parser generated outputs against the correct respective outputs for the example Autolev codes in the test_examples folder. I think this a good way to write tests as it is not easy to write simple one liner unit tests. For example the Kane() command needs the context of the whole system and can't be checked on its own. I'll make sure the Travis build passes. |
sympy/parsing/autolev/Autolev.g4
Outdated
|
||
vec: ID ('>')+ | ||
| '0>' | ||
| '1>>'; |
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.
In here, please add spaces before "|", so that it is aligned with the ":", just like you have done for the expr:
below. I think that seems to be the accepted formatting for the g4 files.
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.
And the same for things above.
@NikhilPappu yes, I meant the antlr generated parser. Yes, it is the same for the latex parser. If you look at the files, they even have a binary part at the beginning (properly encoded as a Python string). I think in general it's best if such files are not checked into the git repository, but rather generated in the release tarball (or on Travis for tests). It does make the git repository not "self-contained" in a sense that you have to have antlr installed in order to generate those, but I think users should use the tarball (or Conda packages, or other packages) all of which would have it generated (since they are usually built from the tarball). /cc @asmeurer Regarding the tests in this PR, I think they way you did it is perfect. I agree with your point. So as far as I am concerned, just do:
|
@certik
Maybe this was a reason it passed on Windows but failed in Travis which uses Unix I think. |
Make sure to use |
sympy/parsing/autolev/__init__.py
Outdated
from sympy.external import import_module | ||
|
||
def parse_autolev(inp, outFile=None): | ||
""" |
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.
Please follow numpydoc standards for docstrings.
sympy/parsing/autolev/__init__.py
Outdated
|
||
from sympy.external import import_module | ||
|
||
def parse_autolev(inp, outFile=None): |
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 a PEP8 linter and follow its suggestions, for example: outFile
-> out_file
.
@@ -0,0 +1,59 @@ | |||
Newtonian N |
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.
Where do these examples come from? We need to assess the copyright and our use of them. Adding them to the sympy repo means they are licensed under the BSD license.
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.
All the examples added till now are from this Autolev Tutorial.
http://web.mae.ufl.edu/~fregly/PDFs/autolev_tutorial.pdf
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 would like to use some examples from Dynamics Online henceforth but as you said previously we might need to alter the code a little due to copyright issues.
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 the tutorial have any copyright information?
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.
@certik Do you have any thoughts on how to handle this? I'm pretty sure both of these sources are not under any kind of open license.
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.
The first page says Copyright 1996-2005 by Paul Mitiguy and Keith Reckdahl. All Rights Reserved.
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.
Yes, regarding the copyright, there are only two ways out. Either
- ask the authors to license the files we use under a BSD license
or
- rewrite the files from scratch (without referencing them), i.e., write them as good tests, without having any similarity to the original files. You can still use the original files on your local computer to ensure it actually works, but in sympy we have to have our own files.
zero = sm.Matrix([i.collect(g)for i in zero]).reshape((zero).shape[0], (zero).shape[1]) | ||
|
||
import scipy.integrate as sc | ||
import pydy.codegen.ode_function_generators as pd |
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 makes pydy a dependency of sympy, no? I don't think that we want any of the numerics based on pydy functions in sympy. SymPy is currently a strict dependency of PyDy, not the other way around. What you can do is generate numeric evaluation results using evalf()
. You can compare the arbitrary precision values.
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 not sure what you mean by using evalf() here. Don't we need to integrate the eoms. Is it fine to use the other imports?
Is it fine to do it like you have done here : http://www.moorepants.info/blog/npendulum.html?
You don't seem to use a PyDy import here.
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.
If you want to check numerical results in SymPy you need to use things like xreplace
, subs
, and evalf
. This calls the underlying mpmath library and outputs arbitrary precision results. For long complicated expressions, evaluating them in both Autolev and SymPy numerically for random inputs is a good way to check the correctness.
You can use lambdify as in the npendulum blog post too, but this will give you floating point precision.
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 haven't written tests to compare the numerical outputs of SymPy and Autolev yet.
The current tests in test_autolev.py and parsing/test_examples just compare the parser generated output against the correct output (the files test_out1-10 contain what I consider the correct outputs) so that if anything is changed one can see if anything is broken.
You can look at the file test_autolev.py and it will become clear how the tests work.
The code generated here isn't for comparing numerical outputs.
The code generated here is the code for Simulation of EOMs after the system has been specified. This is the parsed equivalent of Autolev's Kane(), Input, Output and Code Dynamics() commands.
This is looking great so far!. Some big picture comments:
|
Some clarifications:
What I plan to do next:
|
Regarding the antlr generated files: the problem is that once this is merged, even if the files are removed later, the git repository will still contain them, and thus the size of it will be increased forever. @asmeurer, what do you think we should do here? |
@moorepants I'll change the names of test1-10_out.py to expected_output1-10.txt as the name seems misleading. These are the correct parsed code for the input Autolev codes provided. Not the actual tests. |
@asmeurer @certik
This is passing on my PC when I run bin/test parsing but it is causing Travis errors of this sort:
How can I fix this? How can I access specific files in the SymPy directories? |
You should use |
@moorepants Can you show me an example? Also tests seem to be run using utilities/runtests.py in Travis while I use bin/test on my PC. Would that make any difference? |
@NikhilPappu not sure how to fix it. Try to ask on the mailinglist if anyone knows. |
Also please take a look at the bot status whenever you edit the release notes or before merging to verify that they look correct. In this case, there was a bug in the way it handled the multiple bullet points (which I have fixed at sympy/sympy-bot#15). |
@asmeurer Sorry about the commits Aaron. I shall follow these conventions and the release notes thumb rule from next time for sure. I shall also check the bot status. |
|
||
Parameters | ||
---------- | ||
inp: str |
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.
Why not "input" instead of "inp"?
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.
Sorry I changed that. I thought it was conflicting with Python but it was just the syntax highlighting on my editor.
1. Can be the name of an output file to which the SymPy code should be written to. | ||
2. Can be the string "print". In this case the SymPy code is written to stdout. | ||
3. Can be the string "list". In this it returns a list containing the SymPy code. | ||
Each element in the list corresponds to one line of 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.
What happens when output=None
? Maybe the default should be to stdout.
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 that is better. I'll remove the print option and set the default to stdout.
>>> l.append("INPUT Q1=.1,Q2=.2,U1=0,U2=0)" # doctest: +SKIP | ||
>>> 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 |
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.
Any reason this can't be written like:
>>> """\
... My
... 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.
It can be written in a better way. I shall change it.
kane = me.KanesMethod(frame_n, q_ind=[q1,q2], u_ind=[u1, u2], kd_eqs = kd_eqs) | ||
fr, frstar = kane.kanes_equations([particle_p, particle_r], forceList) | ||
zero = fr+frstar | ||
from pydy.system import System |
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 the PyDy code output should be optional. We are creating a circular dependency of sorts by including it. Either the autolev parser belongs in PyDy or an extension to the parser belongs in PyDy. I think we should stick to outputting only what SymPy can do with SymPy code.
Another option could be a flag: parse_autolev(..., include_pydy=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.
I think adding the flag is a good idea. I shall do that.
from sympy.external import import_module | ||
|
||
|
||
def parse_autolev(inp, output=None): |
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.
What version(s) of autolev code does this parse?
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 followed the Autolev Tutorial and the Autolev version you sent me (these are version 4.1) and Dynamics Online (which uses an older version) as guides. I didn't find too much of a difference when I used the older codes in the newer version but I think I went with the newer version in case of a conflict or deprecation warning. So I would say 4.1.
Also, one has to keep in mind some conventions in some cases while writing Autolev code for the parser for it to work properly (I shall discuss all the nuances in the documentation).
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.
Ok, stating clearly that we are following the autolev 4.1 spec is good.
__import__kwargs={'fromlist': ['AutolevLexer']}).AutolevLexer | ||
AutolevListener = import_module('sympy.parsing.autolev._antlr.autolevlistener', | ||
__import__kwargs={'fromlist': ['AutolevListener']}).AutolevListener | ||
except Exception: |
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 should be a specific exception.
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 copied this style from the LaTeX parser.
I think ImportError should work though.
angvelmat = diffed * dcm2diff.T | ||
# angvelmat = diffed * dcm2diff.T | ||
# This one seems to produce the correct result when I checked using Autolev. | ||
angvelmat = dcm2diff*diffed.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.
This should be in a separate PR with a specific test for it. What is the test case that is broken?
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 shall add a test for it.
There was no test case for this to begin with so I didn't break anything.
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 change should not have been merged. Just wanted to note that this broke working code as seen here: #16824. This PR seemed to have been merged without my concern addressed.
@@ -381,6 +382,55 @@ def gravity(acceleration, *bodies): | |||
|
|||
return gravity_force | |||
|
|||
|
|||
def center_of_mass(point, *bodies): |
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.
Are there any tests for 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 haven't added a test for this. I shall do so in the next 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.
This was also not addressed before merging.
@NikhilPappu I've had some time to look at this. Sorry it is after it was merged. This is really nice work and a great addition. I've left some comments that can be addressed in new PRs. Also, I'm a bit confused on how the testing works. What I imagine as a good test for this is that we write some autolev code, run the autolev code to get its symbolic results, then we run the parser on the autolev code to generate the sympy code, and then run the sympy code to get its symbolic results. Once we have the symbolic results from both programs we can compare them in two ways:
Or
It isn't clear to me if either of these are done. And thus I am not sure how to tell whether the autolev parser generates equivalent code. Can you comment on this? |
@moorepants thanks for the review. Yes, @NikhilPappu please address the stuff in new PRs. To answer your question about tests --- my understanding is that it tests the (current) results from the parser. So if you change some code in the parser, it will break the tests, thus ensuring that the parser works. This has the advantage that it runs quickly (no pydy necessary). If a bug is discovered, i.e., that the tests, as currently written, test an incorrect result, then we correct the test and fix the bug. We can always write slower and more deeper tests to actually run pydy and autoleve side by side and compare the results. But the above seems like an excellent way to test the parser itself. |
I see how these are regression tests. But it isn't clear to me how we know the parser emits correct (symbolic and numerically speaking) code. |
@moorepants, yes, these are regression tests. We do not know that the parser is emitting correct code. In order to know that, we need to finish some tests in the pydy I assume. I would be nice if @NikhilPappu could write such tests also, probably in pydy, before the GSoC is over. At least a few examples. |
I'll discuss with him and I should be able to help with some. |
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.
from sympy.external import import_module | ||
|
||
|
||
def parse_autolev(inp, output=None): |
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 had a new thought about this main parsing function. Most functions in python that operate on text are more versatile if they can take more than a string as an input. For example the pandas read_csv()
function has this primary argument:
filepath_or_buffer : str, pathlib.Path, py._path.local.LocalPath or any object with a read() method (such as a file handle or StringIO)
The string could be a URL. Valid URL schemes include http, ftp, s3, and file. For file URLs, a host is expected. For instance, a local file could be file://localhost/path/to/table.csv
The parser function then outputs a string. This string could be sent to a buffer or file or whatever the user wants. I'm not sure that having the output file as a kwarg is a good idea, because it silos the user into doing one type of thing and isnt' flexible. It basically means that users would only ever want to write a file to disk.
With that said, I think the parser API would be more flexible if it followed these more pythonic conventions.
@certik, do you have any thoughts on this?
We can revert the broken change.
…On Tue, Oct 15, 2019, at 9:46 PM, Jason K. Moore wrote:
***@***.**** commented on this pull request.
In sympy/physics/vector/frame.py <#14758 (comment)>:
> @@ -260,7 +260,9 @@ def _w_diff_dcm(self, otherframe):
from sympy.physics.vector.functions import dynamicsymbols
dcm2diff = self.dcm(otherframe)
diffed = dcm2diff.diff(dynamicsymbols._t)
- angvelmat = diffed * dcm2diff.T
+ # angvelmat = diffed * dcm2diff.T
+ # This one seems to produce the correct result when I checked using Autolev.
+ angvelmat = dcm2diff*diffed.T
This change should not have been merged. Just wanted to note that this broke working code as seen here: #16824 <#16824>. This PR seemed to have been merged without my concern addressed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#14758?email_source=notifications&email_token=AAAFAWDHGR5ALWM7TUIJVB3QO2MEBA5CNFSM4FCPX2S2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCICQN5I#discussion_r335276062>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWCKMH2HYMSEJPHPMJ3QO2MEBANCNFSM4FCPX2SQ>.
|
It was fixed in PR #16828. The issue is that I'm teaching a course with the package now and the bug is present in the latest release of SymPy which is what my students are using. |
@@ -121,6 +121,9 @@ def __init__(self, frame, q_ind, u_ind, kd_eqs=None, q_dependent=None, | |||
u_auxiliary=None): | |||
|
|||
"""Please read the online documentation. """ | |||
if not q_ind: | |||
q_ind = [dynamicsymbols('dummy_q')] | |||
kd_eqs = [dynamicsymbols('dummy_kd')] |
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 change was also introduced with no tests or explanation.
I merged it, I am really sorry that it broke things. I am glad it is fixed. What are the possible paths forward to get it working for your students? I know @asmeurer is trying to do a new release, but that might not be on time for your classes. We can upload a conda package into a special channel just for you, using the latest master. Let me know what you want to do. |
I'm running a new jupyterhub server for the class. I can make a new docker image with the patch to make it available to the students. Don't worry about it. I just wanted to document what happened here and review the other changes. |
Ok, perfect. Thank you.
…On Wed, Oct 16, 2019, at 6:34 AM, Jason K. Moore wrote:
I'm running a new jupyterhub server for the class. I can make a new docker image with the patch to make it available to the students. Don't worry about it. I just wanted to document what happened here and review the other changes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#14758?email_source=notifications&email_token=AAAFAWDBTPHREAP3AWNDY4TQO4J43A5CNFSM4FCPX2S2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBMP3XQ#issuecomment-542703070>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWABRWZRDYHSNPKQP23QO4J43ANCNFSM4FCPX2SQ>.
|
mass of a system of bodies.
to prevent errors which shouldn't occur).