-
Notifications
You must be signed in to change notification settings - Fork 17
Cube Cutout Generalization #146
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
Conversation
a4cd398 to
9fbb5a0
Compare
havok2063
left a comment
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.
Overall looks good to me.
astrocut/CubeCutout.py
Outdated
| @abstractmethod | ||
| def _build_tpf(self, cube_fits: fits.HDUList, cutout: 'CubeCutoutInstance') -> fits.HDUList: | ||
| """ | ||
| Build the cutout target pixel file (TPF). | ||
| This method is abstract and should be defined in subclasses. | ||
| """ | ||
| pass |
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.
Will CubeCutout be used as the parent for any non-TESS cube cutout classes, e.g. JWST or MaNGA cubes? If so, maybe this should be moved to TessCubeCutout, or moved into some intermediate TessBase class if it needs to be an abstractmethod.
astrocut/cube_cut.py
Outdated
|
|
||
| return cutout_hdu_list | ||
|
|
||
| def cube_cut( |
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.
To mirror fits_cut and img_cut, should this be made into a convenience function?
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.
Do you mean to take it out of CubeFactory or add another function entirely? I chose not to take it out of CubeFactory because it wouldn't be backwards compatible.
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 was thinking of taking it out of the class into its own function, but that was primarily to standardize the astrocut API. If you need to keep it for backwards compatibility, that's fine. In that case, we may want to create a wrapper cube_cut function.
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 like this idea! New commit adds a stand-alone cube_cut function.
|
Sorry, I messed up the rebase 😭 This should be the last commit if the CI passes. |
More generalization stuff!!! Only one more to go after this 🎉
A few notes:
TessCubeCutoutthat inherits from abstractCubeCutout. I think I moved all the TESS-specific logic into the child class, but I may have missed some pieces.CubeCutoutInstance, that has helpful attributes for each singular cutout.InvalidInputErrorfor invalid inputs that require little to no processing (not a valid option, not the right data type, etc.).InvalidQueryErrorwill be for calls that do some processing on "valid" inputs, but ultimately fail due to some issue (no data, out of footprint, etc.)