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
Perform corrections to Line Analysis Flux Results #1564
Perform corrections to Line Analysis Flux Results #1564
Changes from 41 commits
88cfc86
e74c0e4
5a3cad4
ab3063a
0438506
a118e86
cac64a6
54b98c4
982a377
e762634
a71301c
f44aa0b
31d2f7b
549299f
9301ec4
cae45bf
d6d5e85
316af5c
ffed328
972a1f4
1c32f30
ac16fd2
23a9f7e
4a7cf97
320f91a
61debce
fda9b00
1aeca73
a4d8ea7
fbbffa5
1e7121a
f6a2529
99fb4fc
c2e1e49
9d34a52
7ac6af8
9380d31
01f60bc
eee47e0
ebbe983
c360db4
f9c513c
31ab2ee
5631edc
f9589a6
1557d16
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.
Because we're doing line flux unit conversion to all data, not just those from
JWST
, it has impacted these tests here. Because these values are marked as "not validated" as per the comment, I just copied over the results from the test and replaced them here, since it is suggested to me that the values don't actually matter. But I don't know if there is a better approach to this. If I know someone who will call me out and tell me there's a better way, it's @pllim !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.
This is where science validation becomes important. Have the POs also test this plugin with some non-JWST data and see if results are correct.
On our side:
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.
Also, do you understand this change? What was the unit before, what is the unit now?
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.
This is definitely a PR that requires PO review, so I agree for the first, and last, point. Though I wonder whether a full scientific integrity test suite should be a blocker for this PR or not?
As for your other point, the main solace I can provide for the "arbitrary data" is that the conversion only happens when the units are specifically in the right bases. Could a user theoretically provide some other data that happens to have "the same unit" but mean something different? I don't know
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.
Well, if we want full QA, this is never going to go in. So I would say no, but we first need to understand why our existing test results have changed and is the change correct.
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.
@pllim (1) Regarding arbitrary units, these shouldn't be touched by this PR. Specutils should know how to handle the units commonly used by astronomers: Jy, erg/s/cm^2/A, etc..., regardless of what instrument/telescope is used.
(2) The easiest meaningful test is a Gaussian with flux = 1 W/m^2. Not necessary to test with real data if this gives the right answer.
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.
Here are the only two cases for spectra with physical units:
(1) energy/time/area/wavelength[/solid_angle] vs. wavelength or frequency---
integrate over wavelength, convert to W/m^2[/sr]
(2) energy/time/area/frequency[/solid_angle] vs. wavelength or frequency---
integrate over frequency, convert to W/m^2[/sr]