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

Updated parser code #15006

Merged
merged 10 commits into from Aug 31, 2018

Conversation

Projects
None yet
4 participants
@NikhilPappu
Copy link
Contributor

NikhilPappu commented Aug 1, 2018

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:

  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 tests.
  • parsing

    • Made changes to _listener_autolev_antlr in parsing.autolev. Revamped the processVariables function
      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.
    • Also changed some code in tests/test_autolev.py (Added commented test code for the GitLab repo
      tests and changed zip to zip_longest which is more correct.
  • physics.mechanics

    • 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 tests.
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Aug 1, 2018

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:

  • parsing

    • Made changes to _listener_autolev_antlr in parsing.autolev. Revamped the processVariables function
      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. (#15006 by @NikhilPappu)

    • Also changed some code in tests/test_autolev.py (Added commented test code for the GitLab repo
      tests and changed zip to zip_longest which is more correct. (#15006 by @NikhilPappu)

  • physics.mechanics

    • 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 tests. (#15006 by @NikhilPappu)

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.

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:
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 tests.

<!-- BEGIN RELEASE NOTES -->
- parsing
   - Made changes to _listener_autolev_antlr in parsing.autolev. Revamped the processVariables function 
     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.
   - Also changed some code in tests/test_autolev.py (Added commented test code for the GitLab repo 
     tests and changed zip to zip_longest which is more correct.

- physics.mechanics
   - 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 tests.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 1, 2018

@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.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Aug 2, 2018

Looks like you found another bug in the bot. Thanks @NikhilPappu!

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Aug 2, 2018

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.

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 2, 2018

@NikhilPappu Here is a nice guide to writing good commit messages: https://chris.beams.io/posts/git-commit/

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Aug 2, 2018

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.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Aug 2, 2018

Also one thing that I would add that doesn't seem to be in either guide is to never use the -m flag to git commit. Let it open your editor so you can write a real message. Using -m nudges you to writing messages that are too short. If you don't like how git commit opens vim when using -m you can configure your EDITOR environment variable to open something better (nano is the simplest option, and most GUI editors have a command line tool that you can set EDITOR to that will open in them).

@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 2, 2018

@asmeurer @moorepants
Sorry about not providing any descriptions. I made the PR in a hurry and just wanted to check the Travis build for once. I will write the PR message and the release notes.
As for multiple PRs, I think the changes made here are minor for the most part and this PR should be enough for the changes in the parser. I think I could open different PRs for the tests added to test_functions.py and test_frame.py in physics/mechanics and vector.
As for the commit message I will change it to a more descriptive one. It's hard to say much in just the title so I shall give a proper description in the body of the commit message.

@NikhilPappu NikhilPappu force-pushed the NikhilPappu:autolev_parser branch 2 times, most recently from a91b1b1 to 6c48670 Aug 2, 2018

Updated the parser code and made changes requested in #14758.
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:
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 NikhilPappu force-pushed the NikhilPappu:autolev_parser branch from 227af54 to 7d99bef Aug 19, 2018

@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 19, 2018

@moorepants I have changed the parse_autolev api and added a warning for the keywords as per your suggestion in the documentation PR.

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 21, 2018

@NikhilPappu resolve the merge conflicts.

NikhilPappu added some commits Aug 21, 2018

@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 21, 2018

@moorepants I resolved them.

@moorepants
Copy link
Member

moorepants left a comment

Some minor comments added. I'm going to also test this locally and will give any feedback if needed.

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...

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

This is a dangling sentence, make complete.

INPUT Q1=.1,Q2=.2,U1=0,U2=0
INPUT TFINAL=10, INTEGSTP=.01
CODE DYNAMICS() some_filename.c
------------FILE--------------

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

Does this render correctly when parsed? Don't you need a :: or .. code: XXX?

>>> 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

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

Should use:

with open("double_pendulum.al", 'r') as f:
    parse_autolev(f, include_numeric=True)

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

Don't skip the doctest if possible.

Load the text like:

>>> my_al_text = """\
stuff
more stuff
another thing"""

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 30, 2018

Author Contributor

I have changed the doctest and the test passes locally but Travis won't pass it because of some antlr import stuff.

This comment has been minimized.

@moorepants

moorepants Aug 30, 2018

Member

Does antlr need to be added to the .travis.yml file?

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 30, 2018

Author Contributor

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.

This comment has been minimized.

@moorepants

moorepants Aug 30, 2018

Member

Is it because antlr is not installed in the doc tests section on travis?

This comment has been minimized.

@moorepants

moorepants Aug 30, 2018

Member

Open an issue for this "antlr not available for doctests".

This comment has been minimized.

@asmeurer

asmeurer Aug 30, 2018

Member

Use the doctests_depends_on decorator to skip the doctest if a module isn't installed.

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)

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

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.

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

An issue should be opened about supporting config constraints, auxiliary speeds, etc.

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 30, 2018

Author Contributor
@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 29, 2018

First error I got when trying locally is:

/home/moorepants/src/sympy/sympy/external/importtools.py:145: UserWarning: antlr4 module is not installed
  warnings.warn("%s module is not installed" % module, UserWarning)
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-3-7dc7f7398af9> in <module>()
      1 with open('whipple.al', 'r') as f:
----> 2     res = parse_autolev(f)
      3 

~/src/sympy/sympy/parsing/autolev/__init__.py in parse_autolev(autolev_code, include_numeric)
     97 
     98     if _autolev is not None:
---> 99         return _autolev.parse_autolev(autolev_code, include_numeric)

~/src/sympy/sympy/parsing/autolev/_parse_autolev_antlr.py in parse_autolev(autolev_code, include_numeric)
     17     antlr4 = import_module('antlr4', warn_not_installed=True)
     18     if not antlr4:
---> 19         raise ImportError("Autolev parsing requires the antlr4 python package,"
     20                           " provided by pip (antlr4-python2-runtime or"
     21                           " antlr4-python3-runtime) or"

ImportError: Autolev parsing requires the antlr4 python package, provided by pip (antlr4-python2-runtime or antlr4-python3-runtime) or conda (antlr-python-runtime)

I then tried:

$ conda search antlr
moorepants@garuda:~$ conda search antlr
Loading channels: done
# Name                  Version           Build  Channel             
antlr                     2.7.7               2  conda-forge         
antlr                     2.7.7               3  conda-forge         
antlr                     2.7.7               4  conda-forge         
antlr                     2.7.7               5  conda-forge         
antlr                     2.7.7               6  conda-forge         
antlr                     2.7.7               7  conda-forge         
antlr                     2.7.7      hfc679d8_7  conda-forge         
antlr                     2.7.7          py27_0  conda-forge         
antlr                     2.7.7          py27_1  conda-forge         
antlr                     4.7.1               0  conda-forge

There does not seem to be any package called antlr-python-runtime in the conda channels defaults of conda-forge.

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 29, 2018

I take that back. It is just conda's poor search command:

moorepants@garuda:~$ conda search antlr*
Loading channels: done
# Name                  Version           Build  Channel             
antlr                     2.7.7               2  conda-forge         
antlr                     2.7.7               3  conda-forge         
antlr                     2.7.7               4  conda-forge         
antlr                     2.7.7               5  conda-forge         
antlr                     2.7.7               6  conda-forge         
antlr                     2.7.7               7  conda-forge         
antlr                     2.7.7      hfc679d8_7  conda-forge         
antlr                     2.7.7          py27_0  conda-forge         
antlr                     2.7.7          py27_1  conda-forge         
antlr                     4.7.1               0  conda-forge         
antlr-python-runtime           4.7.1          py27_0  conda-forge         
antlr-python-runtime           4.7.1          py35_0  conda-forge         
antlr-python-runtime           4.7.1          py36_0  conda-forge         
@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 29, 2018

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@garuda:sympy(pr-15006)$ ipython
Python 3.5.5 | packaged by conda-forge | (default, Jul 23 2018, 23:45:43) 
Type 'copyright', 'credits' or 'license' for more information
IPython 6.5.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from sympy.parsing.autolev import parse_autolev

In [2]: with open('whipple.al', 'r') as f:
   ...:     res = parse_autolev(f)
   ...:     
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-2-7dc7f7398af9> in <module>()
      1 with open('whipple.al', 'r') as f:
----> 2     res = parse_autolev(f)
      3 

~/src/sympy/sympy/parsing/autolev/__init__.py in parse_autolev(autolev_code, include_numeric)
     97 
     98     if _autolev is not None:
---> 99         return _autolev.parse_autolev(autolev_code, include_numeric)

~/src/sympy/sympy/parsing/autolev/_parse_autolev_antlr.py in parse_autolev(autolev_code, include_numeric)
     35         my_listener = MyListener(include_numeric)
     36         walker = antlr4.ParseTreeWalker()
---> 37         walker.walk(my_listener, tree)
     38         return "".join(my_listener.output_code)

~/miniconda3/lib/python3.5/site-packages/antlr4/tree/Tree.py in walk(self, listener, t)
    149         self.enterRule(listener, t)
    150         for child in t.getChildren():
--> 151             self.walk(listener, child)
    152         self.exitRule(listener, t)
    153 

~/miniconda3/lib/python3.5/site-packages/antlr4/tree/Tree.py in walk(self, listener, t)
    149         self.enterRule(listener, t)
    150         for child in t.getChildren():
--> 151             self.walk(listener, child)
    152         self.exitRule(listener, t)
    153 

~/miniconda3/lib/python3.5/site-packages/antlr4/tree/Tree.py in walk(self, listener, t)
    150         for child in t.getChildren():
    151             self.walk(listener, child)
--> 152         self.exitRule(listener, t)
    153 
    154     #

~/miniconda3/lib/python3.5/site-packages/antlr4/tree/Tree.py in exitRule(self, listener, r)
    165     def exitRule(self, listener:ParseTreeListener, r:RuleNode):
    166         ctx = r.getRuleContext()
--> 167         ctx.exitRule(listener)
    168         listener.exitEveryRule(ctx)
    169 

~/src/sympy/sympy/parsing/autolev/_antlr/autolevparser.py in exitRule(self, listener)
    556         def exitRule(self, listener):
    557             if hasattr(listener, "exitVecAssign"):
--> 558                 listener.exitVecAssign(self)
    559 
    560 

~/src/sympy/sympy/parsing/autolev/_listener_autolev_antlr.py in exitVecAssign(self, ctx)
   1490 
   1491                     if v1 == "p":
-> 1492                         if self.type2[v2] == "point":
   1493                             e2 = self.symbol_table2[v2]
   1494                         elif self.type2[v2] == "particle":

KeyError: 'no'
@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 29, 2018

@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.

@asmeurer asmeurer added this to the SymPy 1.3 milestone Aug 29, 2018

@@ -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())])

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

I think xreplace() should, in general, be used instead of subs.

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

This also needs a unit test.

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

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.

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 30, 2018

Author Contributor

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.

This comment has been minimized.

@moorepants

moorepants Aug 30, 2018

Member

This gets exacerbated on larger problems.

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 30, 2018

Author Contributor

I am not sure about a workaround. I shall move this to another PR then.

# 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)

This comment has been minimized.

@moorepants

moorepants Aug 29, 2018

Member

Don't leave commented code in like this.

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 30, 2018

Author Contributor

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.

This comment has been minimized.

@moorepants

moorepants Aug 30, 2018

Member

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.

This comment has been minimized.

@NikhilPappu

NikhilPappu Aug 30, 2018

Author Contributor

That sounds better. I shall change it.

Made the requested changes
Also fixed the position vector error observed in #15165.
@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 30, 2018

@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.

@NikhilPappu NikhilPappu reopened this Aug 30, 2018

... 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"

This comment has been minimized.

@moorepants

moorepants Aug 30, 2018

Member

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??

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 30, 2018

Looks like you need something that says "if using symengine" use subs, otherwise use xreplace.:

___________ sympy/physics/mechanics/tests/test_kane.py:test_one_dof ____________
Traceback (most recent call last):
  File "sympy/utilities/runtests.py", line 1277, in _timeout
    function()
  File "/home/travis/build/sympy/sympy/sympy/physics/mechanics/tests/test_kane.py", line 40, in test_one_dof
    KM.mass_matrix_full.LUsolve(KM.forcing_full)) == zeros(2, 1)
  File "sympy/physics/mechanics/kane.py", line 641, in forcing_full
    return -Matrix([f1.xreplace(self.kindiffdict()), self._f_d.xreplace(self.kindiffdict()), self._f_dnh.xreplace(self.kindiffdict())])
AttributeError: 'symengine.lib.symengine_wrapper.MutableDenseMatrix' object has no attribute 'xreplace'
@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 30, 2018

Opened issue here: symengine/symengine#1456

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 30, 2018

I'm fine with merging. Let me know when tests pass and last things are done.

@NikhilPappu NikhilPappu force-pushed the NikhilPappu:autolev_parser branch from 271fd6b to 6405521 Aug 30, 2018

@NikhilPappu NikhilPappu force-pushed the NikhilPappu:autolev_parser branch from 6405521 to 73333b5 Aug 31, 2018

@NikhilPappu

This comment has been minimized.

Copy link
Contributor Author

NikhilPappu commented Aug 31, 2018

@moorepants I think this can be merged now.

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Aug 31, 2018

Great!

@moorepants moorepants merged commit ceff9f6 into sympy:master Aug 31, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
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.