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

Don't change the case of autolev variables #15166

Open
moorepants opened this issue Aug 29, 2018 · 15 comments · May be fixed by #16580 or #26441
Open

Don't change the case of autolev variables #15166

moorepants opened this issue Aug 29, 2018 · 15 comments · May be fixed by #16580 or #26441
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

Currently the autolev parser converts all autolev variables into lower case python equivalents. This introduces cognitive overload for the reader of the two code versions. Autolev is case insensitive but an author of the autolev code may (and is likely) to use case when writing the code to help with readability. When we convert everything to lower case it is hard to read both sets of code and check for errors etc. We should not change the case of the Autolev code (even though python style guides recommend lower case variables). This could be an option in the parser, but the default should be to not change case.

@NikhilPappu do you have thoughts on this?

@NikhilPappu
Copy link
Contributor

@moorepants Many Autolev examples I came across were in all caps so I thought it wouldn't look good in Python. I agree with this though. Adding an option would be the best way to go.

@moorepants moorepants added the Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. label Aug 29, 2018
@moorepants
Copy link
Member Author

Ok, so maybe something like:

parse_autolev(al_code, lower_case=True)

Gonna add "easy to fix" on this one. Might be good PR for a newcomer.

@CodHeK
Copy link

CodHeK commented Aug 29, 2018

@moorepants I'd like to add this functionality, I am not very thorough with the code base yet so could you guide me with what auto lev parser is or if you tell me where the files for it is , I check physics/mechanics didn't find it any where.
Any help would be appreciated, Thanks!

@moorepants
Copy link
Member Author

The files are in sympy/parsing/autolev. @NikhilPappu can point you more directly to the relevant files.

@CodHeK
Copy link

CodHeK commented Aug 30, 2018

Alright, can i work on this issue ? @moorepants

@moorepants
Copy link
Member Author

You don't need to ask if you can work on the issue. Just say you are and do it.

@NikhilPappu
Copy link
Contributor

NikhilPappu commented Aug 30, 2018

@CodHeK
The changes you would need to make are:

  1. Add the parameter lower_case in parse_autolev() and in the Listener class in the files __init__.py, _parse_autolev_antlr.py and _listener_autolev_antlr.py. These files belong to sympy/parsing/autolev.
  2. Go over all the .lower() occurences in _listener_autolev_antlr.py (there's a lot of them) and replace the ones which are meant for the output and not for internal comparison.
  3. Add lower_case=True in the parse_autolev() call in sympy/parsing/tests/test_autolev.py so that the current tests pass.

@CodHeK
Copy link

CodHeK commented Aug 30, 2018

@NikhilPappu Alright I'll make the following changes. Is there any way I could test it ?

@NikhilPappu
Copy link
Contributor

@moorepants @CodHeK Now that I took a closer look at this, I feel it isn't that easy to fix for someone else. I think I shall fix it myself and maybe update it so that it is easier to make changes from now.

@CodHeK
Copy link

CodHeK commented Aug 30, 2018

@NikhilPappu The Changes to .lower() need to made inside the class MyListener(AutolevListener) in the file _listener_autolev_antlr.py right ?
and only to the ones where it outputs that is in the self.write() calls?

@NikhilPappu
Copy link
Contributor

@CodHeK Technically yes but there is a lot more to it. For instance, you would need to use .lower() in all dictionaries in the code to preserve the case insensitivity of the parser.

@Sanjay126
Copy link

@NikhilPappu @moorepants I am working on this issue and want to completely understand this parser as I am willing to extend the parser for other languages as a project for GSOC'19.

@abhaydhiman
Copy link
Contributor

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

@aquazod
Copy link

aquazod commented Mar 29, 2024

Hello,
I would like to work on this issue.

@aquazod aquazod linked a pull request Apr 2, 2024 that will close this issue
@aquazod
Copy link

aquazod commented Apr 20, 2024

@moorepants Hello Jason, hopefully you are fine!
Can you please check my pull request, it passed the required checks for merging, I can work on the optional dependencies if needed.
Thank you in advance!

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