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

Improve variable naming in autolev parser #15187

Closed
moorepants opened this issue Sep 5, 2018 · 13 comments · Fixed by #21283
Closed

Improve variable naming in autolev parser #15187

moorepants opened this issue Sep 5, 2018 · 13 comments · Fixed by #21283
Labels
Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. parsing physics.mechanics

Comments

@moorepants
Copy link
Member

Here is an example output from #15165:

import sympy.physics.mechanics as me
import sympy as sm
import math as m
import numpy as np

frame_c = me.ReferenceFrame('c')
frame_d = me.ReferenceFrame('d')
frame_f = me.ReferenceFrame('f')
fd, dc = me.dynamicsymbols('fd dc')
fdd, dcd = me.dynamicsymbols('fd dc', 1)
fdd2, dcd2 = me.dynamicsymbols('fd dc', 2)
r, l = sm.symbols('r l', real=True)
point_o = me.Point('o')
point_e = me.Point('e')
frame_d.orient(frame_f, 'Axis', [fd, frame_f.x])
frame_c.orient(frame_d, 'Axis', [dc, frame_d.y])
frame_c.set_ang_vel(frame_f, (frame_c.ang_vel_in(frame_f)).express(frame_f))
point_e.set_pos(point_o, r*frame_d.y-l*frame_c.x)
point_e.set_pos(point_o, (point_e.pos_from(point_o)).express(frame_d))
point_e.set_vel(frame_f, ((point_e.pos_from(point_o)).dt(frame_f)).express(frame_d))
point_e.set_acc(frame_f, ((point_e.vel(frame_f)).dt(frame_f)).express(frame_d))

Name clashes will vary likely happen with the primary import names: me, sm, np. There should at least be changed to _me, _sm, _np and even better there should be a check to ensure the Autolev code doesn't contain and reserved words and the properly handles them by adding underscores or some kind of pre/post fix. For example in autolev you might declare MASS ME as the mass of body E. This will clash with the output code me.

The automatic declaration of the time derivatives is currently:

fd, dc = me.dynamicsymbols('fd dc')
fdd, dcd = me.dynamicsymbols('fd dc', 1)
fdd2, dcd2 = me.dynamicsymbols('fd dc', 2)

A simple improvement would be:

fd, dc = me.dynamicsymbols('fd dc')
fd_d, dc_d = me.dynamicsymbols('fd dc', 1)
fd_dd, dc_dd = me.dynamicsymbols('fd dc', 2)

Where _d, _dd, _ddd, etc is dot, double dot, triple dot, etc.

@moorepants moorepants added Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. physics.mechanics parsing labels Sep 5, 2018
@NikhilPappu
Copy link
Contributor

This should be an easy fix ideal for newcomers. Not so sure about #15166 though.

@ayushmankoul
Copy link
Contributor

Hello @NikhilPappu @moorepants I would like to give this bug a try.Could you please guide me how should I proceed over it ? Thank You

@NikhilPappu
Copy link
Contributor

@ayushmankoul
The code is in sympy/parsing/autolev/_listener_autolev_antlr.py.
For starters prefix all occurences of "me., "sm., "np. and "d" with underscores.
Also, in str_func(z), change "d" + str(z) to "d" * str(z).
Once this is done you will have to update the tests in test_examples (no need to do this manually. I will let you know later how to update them by executing the test script).

@ayushmankoul
Copy link
Contributor

@NikhilPappu
Thank You for guiding me.I have replaced all the instances of me., sm., np. and "d" with underscores in prefix and altered the str_func(z) too.

@NikhilPappu
Copy link
Contributor

@ayushmankoul Go to parsing/tests/test_autolev.py. In the function _test_examples, replace the with open(correct_file_path... with

with open(correct_file_path, "w") as f:
        f.write(generated_code)

Then run bin/test parsing. After this revert the changes made to test_autolev.py and open a pull referencing this issue.

@ayushmankoul
Copy link
Contributor

Sure,I have altered the _test_examples in test_autolev.py and has successfully passed the bin/test parsing for the amendments and reverted the changes back.Please let me know if anything else needs to be done in this issue.Thank You ! @NikhilPappu

@abhijeet47
Copy link

I would like to work on this issue.

@Phantsure
Copy link

well in the previous build only travis ci failed rest there has been all corrections, so i dont know if it would be a good idea to make a new pr

@arooshiverma
Copy link
Contributor

Is anyone working on this one? Can I work on this issue?

@Preet538-neitzen
Copy link

If this has not been resolved yet, I would like to give it a try.

@abhaydhiman
Copy link
Contributor

@NikhilPappu Can you tell me what exact values does the ctx takes in _listener_autolev_antlr.py file.

@abbyjng
Copy link
Contributor

abbyjng commented Apr 5, 2021

If this issue has not yet been resolved, I would like to work on it. I'm new to contributing; could someone give me pointers to where to start?

@abbyjng
Copy link
Contributor

abbyjng commented Apr 10, 2021

I have created a PR for this issue at #21283. I would appreciate it if someone could look over that PR for me and give me feedback. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. parsing physics.mechanics
Projects
None yet
9 participants