Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add footprint finder code #127
base: main
Are you sure you want to change the base?
Add footprint finder code #127
Changes from all commits
2c61ca6
c6b3e0b
509d766
ccb34ca
9726dc3
2b29770
3471090
735112e
a9e0de7
2f90a1e
31926a4
3a0a513
3fb0051
00ab68e
331148b
9552cbb
5e2f889
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 may want to make an issue or note to come back to this and generalize this somehow so it could be used for other missions. i think it could wait for now though, as I think TESS is the only mission we'd do this for.
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.
Are there some situations where someone might still want to make cutouts from a different storage path?
Could we make this an option?
One instance where someone might want a different option is if they have downloaded the cube, to make direct cutouts. But in that case, maybe they are just directly using
cube_cut
?Another option might be if they've mounted stpubdata cube data onto their machine or cloud environment (w/ fuse or something else) in which case they'd rather use that path than the s3 path. E.g. TIKE cloud platform?
I do think we can probably default to these paths, though.
Or maybe we just indicate that this function is only for making cutouts from s3 files? I guess it's already in the docstring, but it's not obvious from the module name or the function name.
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.
My thought was that users would use
cube_cut
in the case that they already have the path (whether local or cloud) to a single cube file. I think that providing a single file kind of defeats the purpose of the footprint lookup.A mounted filesystem or a local path to many cube files is worth considering, but I do wonder how common that use case would be. Could we guarantee that the cube files match the footprints coming from S3?
I'm inclined to rename the function to something like
s3_cube_cut_from_footprint
and make a new issue to explore other options at a later time.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.
maybe
cloud_cube_cut_from_footprint
though it is a bit long?though thinking more on it, i think what you have also works. i don't think there's a need to make an issue now. we can wait until a use case comes up.
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 think we could abbreviate and use
cloud_cube_cut_from_fp
? That may cause some confusion though since "fp" isn't too obvious.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.
between the two, i think i'd prefer just leaving off
cloud
. i'm not a big fan of acronym andfp
isn't obvious