-
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
Make cutouts out of TICA cubes #60
Conversation
Codecov ReportBase: 94.85% // Head: 93.78% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
- Coverage 94.85% 93.78% -1.08%
==========================================
Files 8 8
Lines 1107 1415 +308
==========================================
+ Hits 1050 1327 +277
- Misses 57 88 +31
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Note: |
Tests will continue to fail until PR #59 gets merged and this branch gets updated with those changes. |
639c573
to
09ad86c
Compare
…or TICA; added TODO item to generalize check1;
|
@@ -84,27 +89,34 @@ def __init__(self): | |||
"VIGNAPP": [None, "vignetting or collimator correction applied"]} | |||
|
|||
|
|||
def _parse_table_info(self, table_data, verbose=False): | |||
def _parse_table_info(self, product, table_data, verbose=False): |
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.
we do use this function in tesscut. perhaps we shouldn't be, since it's "internal". or perhaps it should not be an internal function. nevertheless, it's fine as you have it, but it might have been nice to have had product be a keyword argument that defaulted to SPOC instead of being a positional arg so that downstream code wouldn't have to change. I believe you are also taking care of the downstream code, at least in tesscut. Are there any other packages that might use this function? Astroquery?
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'm fine with not changing this now. just pointing it out that we need to change it in tesscut (also in the s3_support branch, when we switch over to using this)
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'm in favor of changing this here to prevent any future bugs elsewhere. I'll work on this next.
EDIT: After second thought, it might be better to leave product
as a required positional argument. We would still have to remember to change product to the Enum call wherever this function is on TESSCut, because we can't have SPOC as the default for both SPOC and TICA. And a "missing positional keyword argument" error is more straightforward to fix than whatever error would come out of attempting to parse SPOC info in a TICA table (it would probably be a missing keyword error, but still).
And no _parse_table_info
doesn't seem to be explicitly used in astroquery
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, i think just a few small things here and we are ready!
Co-authored-by: Ben Falk <falk.ben@gmail.com>
4d11e4d
to
1275009
Compare
Implements a class based off the existing CutoutFactory class that can generate TPFs out of TICA cubes.
Note: This is one of the core four projects for MAST Q3 and must be implemented before June.