-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Adding HAPCut functionality to astroquery.mast.Cutouts
tool
#2613
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2613 +/- ##
==========================================
- Coverage 68.82% 64.17% -4.65%
==========================================
Files 294 130 -164
Lines 22215 16891 -5324
==========================================
- Hits 15289 10840 -4449
+ Misses 6926 6051 -875 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
03d3f67
to
fe6ecc9
Compare
This question came up during one of our stand-ups: When will the next release of |
The next tagged release is not planned before we finish most of the API cleanups we planned to do, so maybe Q1 (calendar year) next year. In the meantime I can push dev releases to pypi without any problems. |
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 have some big picture comments, which certainly points beyond this current PR.:
- I wonder at what point will it make sense to make a generic mast.Cutout class, that takes the coordinate and returns all available cutouts, or maybe also takes a missing name? Even if it's not exposed to the public API, I think we reached a point where having mast cutout base class would be beneficial as some of the methods share significant amount of code (e.g. enhancements and bugfixes, or any maintenance will be easier with a baseclass).
Otherwise I have minor comments, so would only like to see a few small things to be fixed.
@@ -1120,6 +1120,62 @@ To list the available deep field surveys at a particular location there is `~ast | |||
['candels_gn_60mas', 'candels_gn_30mas', 'goods_north'] | |||
|
|||
|
|||
HAPCut |
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.
side note: the doctest are failing in an earlier example, so these has not been run during my testing
This has actually been a topic of discussion recently, and we do have plans to consolidate all the cutouts classes into a single mission-agnostic cutout tool. I'm not sure what the timeline will be on this, though. We have a few more things we want to update |
Sounds all great. It certainly points beyond this current PR, and any timeline will work that works for you. |
Good to know, I'll relay this to the team |
Could you squash out some of the later debugging/incremental commits? I plan to come back and go through it again in the evening, but don't think there is any more things to address for merge. |
Yeah, I'm waiting for a science review on this from one group, once I hear back from them I'll squash this down so it's ready for merge. |
b35844f
to
926d2db
Compare
Squashed! @bsipocz |
Thanks! No need to squash to one, it's really more about keeping the incremental fixes at the later parts into more logical chunks. |
926d2db
to
e773121
Compare
Thank you @jaymedina! |
This PR will expand
astroquery.mast
's cutout functionality to support making Hubble Advanced Products (HAP) cutouts via MAST'sHAPCut
API.! Timeline: ASAB would like to see this merged before the end of Q1. Ideally before Dec 21st if possible.
Changes include:
HapcutClass
incutouts.py
HapcutClass
HapcutClass
functionalityExample
download_cutouts
call with sample cutout shown:Example
get_cutouts
call with sample cutout shown(default cutout size is 5 x 5 pixels):