-
Notifications
You must be signed in to change notification settings - Fork 11
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
Remove unused functionality and tests #286
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.
When looking at the deletions, I see at least 5 (I stopped here) modules being removed that are used in one of our repositories:
createAmp.py
: https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/rtc/rtc_sentinel.py#L20make_cogs.py
: https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/rtc/rtc_sentinel.py#L25raster_boundary2shape.py
: https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/rtc/rtc_sentinel.py#L26rtc2color.py
: https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/rtc/rtc_sentinel.py#L27system.py
: https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/rtc/rtc_sentinel.py#L28
We should revisit the methodology used to determine what modules/functions are used across the orgs.
Thanks for the catch @jhkennedy. My script failed to find all the results because of this bug in the GitHub CLI. |
Everything has been added back! |
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 caught one more removed-but-used module
hyp3lib/par_s1_slc_single.py
Outdated
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.
This module is used in hyp3-gamma here:
https://github.com/ASFHyP3/hyp3-gamma/blob/develop/hyp3_gamma/insar/ifm_sentinel.py#L20
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.
Notably, ifm_sentinel.py
also had many hyp3lib
imports
hyp3lib/cutGeotiffs.py
Outdated
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 looks like this is mentioned in insarlab/MintPy documentation (the only hit for hyp3lib):
https://github.com/search?q=org%3Ainsarlab%20hyp3lib&type=code
I don't think that's enough to keep the file, but we'll want to make a PR there to change it -- maybe as part of this (?): insarlab/MintPy#1004
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.
Yep, that's bad information anyway. I think we should remove the functionality and change the docs.
setup.py
Outdated
'extendDateline.py = hyp3lib.extendDateline:main', | ||
'geotiff_lut.py = hyp3lib.geotiff_lut:main', | ||
'get_bounding.py = hyp3lib.get_bounding:main', | ||
'getDemFor.py = hyp3lib.getDemFor:main', | ||
'get_asf.py = hyp3lib.get_asf:main', |
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.
What's still using get_asf.py
?
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.
Similarly, what's still using metadata.py
?
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.
Yep, both of those can be removed.
@jhkennedy this is ready for your re-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.
Everything looks good now! PyCharm inspection also doesn't find anything worrying specific to these changes (thought here's a lot we should clean up):
@forrestfwilliams did you update the PR comment to reflect the final changes?
Yep, updated! |
This PR will be the first of several, which will attempt to modernize hyp3lib for its current usage. This first PR removes all functionality not currently being using by one of these GitHub orgs:
ASFHyP3, asfadmin, ASFOpenSARLab, access-cloud-based-insar, dbekaert, ASFBinderRecipes
All code in the ASFHyP3/scratch directory was excluded.
All of the remaining tests are passing.
Removed files include: