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 3 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.
Maybe you were trying to generalize this but I'm not sure what sequences are. Are these sectors?
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.
Yes, they refer to sectors! I was trying to make the parameter more general and borrowed "sequence" from the CAOM field descriptions: https://mast.stsci.edu/api/v0/_c_a_o_mfields.html
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 guess for a user, it might be a little unclear that for TESS this is sectors, and not cameras or ccds or anything like that. so maybe some documentation or examples would help here.
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.
Added some more info and an example to the docstring in the latest commit! I'll also be updating the documentation at some point (probably next sprint) and will definitely include examples there.
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 am not sure threads here will help, and it may in fact hurt.
cube_cut
already uses threads for accessing the S3 data, so with this change, each cutout file would spawn that many threads. In my testing, there are diminishing returns after 8 threads. Since this could end up creating many times that number of threads, I expect we'd see thread contention here.If you set threads to 0, versus setting threads to "auto" or "8" what are the results here?
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.
Testing on my machine, I do see a performance improvement with a larger number of threads. It's more apparent when a sector isn't provided and more than 1 cutout is being generated. For example, these commands each generate 7 cutouts.
cube_cut_from_footprint('130 30', cutout_size=50, threads=0)
--> 1 min, 23.4 seccube_cut_from_footprint('130 30', cutout_size=50, threads=8)
--> 57.6 seccube_cut_from_footprint('130 30', cutout_size=50, threads='auto')
--> 46.4 secThere 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.
is this before or after you made the change to use the same threads variable to pass into the
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.
Looks like I ran the test before, when
threads
was set to auto forcube_cut
. When using the samethreads
variable, the call withthreads=0
takes a lot longer, which makes sense. I also see that there is less than a second difference betweenthreads=8
andthreads=auto
.Maybe it would be best to keep
threads
forcube_cut
constant at some value, like 'auto' or 8? I think that using threads incube_cut_from_footprint
is still worthwhile for the performance improvement when making several cutouts at once, but performance does seem to stagnate after a certain point.I'm also thinking that the default value for
threads
incube_cut_from_footprint
should be set to 8 rather than 1, since performance is consistently better.