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

NPI-3655 Improve readability of getVelPoly() function #62

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

treefern
Copy link
Collaborator

No functional changes, but a bunch of renamed variables to hopefully make it clearer what this function is doing.

@treefern treefern requested a review from seballgeyer December 18, 2024 07:34
Copy link
Collaborator

@seballgeyer seballgeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good idea to make it clearer.
However sometimes long variable name can make the code more difficult to read. And here some a quite long.

I did a few suggestions, I didn't went through all changes.

Otherwise it is OK

gnssanalysis/gn_io/sp3.py Outdated Show resolved Hide resolved
gnssanalysis/gn_io/sp3.py Outdated Show resolved Hide resolved
gnssanalysis/gn_io/sp3.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@seballgeyer seballgeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More a look into this function, obscurer it looks...
it seems that the 20 lines functions is doing something that numpy can do by itself in 2 or 3 function calls.

I would suggest to merge this PR and eventually rewrite this function later. The objectives of renaming some variable to improve understanding has been achieved.

@treefern treefern merged commit 9d0d8ea into main Dec 19, 2024
4 checks passed
@treefern treefern deleted the NPI-3655-improve-readability branch December 19, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants