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
JP-3768: Move outlier detection median computers to stcal #292
JP-3768: Move outlier detection median computers to stcal #292
Changes from 7 commits
66c5c61
8c8ef11
ddf9d3b
cf557b9
afe4b08
83a8f1f
d41546a
31adf16
509a6ae
19ac974
fafb5e7
c38478a
10378bd
4a350c4
2608a37
c2d10b7
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.
What's the largest index of
shape[0]
tested? When testing ramp fitting the filejw02589006001_04101_00001-seg001_nrs1_uncal.fits
has 6103 integrations. This seriously slowed down computation time in ramp fitting when running python compared to data with the same dimensions, but a much lower number of integrations.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.
If I understand the question you are asking about the runtime performance of the nanmedian3D function, right? This conversation on the JWST PR may be relevant, although no test was done with the zeroth (time/n_groups) axis being as large as that.
When you say that it seriously slowed down runtime in ramp fitting, do you mean that a median calculation was done, or just that the entire step scaled poorly with the number of integrations?
For imaging data, where we've seen the slowest processing, the largest n_groups we have seen is only ~100, since the step takes _cal files as input instead of _calints. However, I can look into whether there would be any coronagraphic data that had a huge number of integrations in their _calints files, as this is also used by jwst for coronagraphic data
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 tried this on a relatively large coronagraphy dataset I had lying around from JWSTDMS-921. The shape of the array going into this function was (1250, 224, 288), so still not 6000 but this is on the large side of what is processed here in practice. The median calculation does not blow up the runtime in this case: the entire outlier detection step took only 5 seconds to run, as compared with ~1 hour for the coronagraphy-specific
align_refs
andklip
steps. So I don't think this is a concern for runtime but let me know if you would like to see additional tests run.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.
Missing docstring describing method.
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.
Let me know if the updated docstring looks ok to you
Check warning on line 102 in src/stcal/outlier_detection/median.py
Codecov / codecov/patch
src/stcal/outlier_detection/median.py#L101-L102
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.
Missing docstring describing method.
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.
Let me know if the updated docstring looks ok to you