-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cleaning and formating files in the ICEsat2_SI_tools folder #132
Conversation
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.
Nice! I've got a few comments and suggestions 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.
I added a number of refactorings and suggestions.
…at2-tracks into clean-files-tools
@cpaniaguam @hollandjg Check my replies on the opened conversations and do a last review. |
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.
Thanks for this! It looks like there are more changes here than just cleaning and formatting – some of the changes to functions look like they need more detailed tests before we can make them.
Is it possible to split this into a PR which does just cleanup, and another (or several) which make functional changes?
src/icesat2_tracks/analysis_db/A01b_ALT07_SHNH_variance_tester.py
Outdated
Show resolved
Hide resolved
@hollandjg I removed the latest changes in |
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.
Looks good to me!
@cpaniaguam one last review on your side? |
"getmem >=1.0, <= 1.0.0", | ||
"astropy >=5.2, <= 6.0.0" |
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 were these added? Just curious.
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.
Removing conditional imports to the top of the files make the interpreter loom for those dependencies when running the scripts
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.
Just a couple more easy changes and a suggested issue.
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's a big change, and it looks good to me. Hopefully nothing has gotten through the cracks.
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.
👍
@cpaniaguam I added few changes requested in old conversations. Some of the doesnt seem to be reflected in their respective conversation. Please re-review and approve if you think is ready. |
self.Lpoints = Lpoints | ||
self.ov = int(Lpoints/2) if ov is None else ov #when not defined in create_chunk_boundaries then L/2 | ||
self.Lpoints = Lpoints | ||
self.ov = int(Lpoints / 2) if ov is None else ov |
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 see it!
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.
Great! Let's merge it!
No description provided.