Add Direct Image to expected file list for NIRCam #1948
Conversation
rosteen
requested review from
duytnguyendtn,
javerbukh,
ojustino,
pllim,
kecnry and
bmorris3
as code owners
Codecov ReportBase: 91.79% // Head: 91.78% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1948 +/- ##
==========================================
- Coverage 91.79% 91.78% -0.01%
==========================================
Files 140 140
Lines 14950 14954 +4
==========================================
+ Hits 13723 13726 +3
- Misses 1227 1228 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Technically a bug? I think this needs a change log. Thanks! |
|
I'll add a changelog if/when it turns out to actually solve the problem, not 100% sure yet. Might need a little more work. |
|
|
|
@camipacifici it turns out that the wrong FITS header keywords were being used for the filter to match between the 2D spectrum and direct image ("PUPIL" for NIRISS vs "FILTER" for NIRCam). The direct image loads now:
|
|
Why is there "deg" in the image axis labels? Should be "pix" right? |
Hm, good question. I wonder if it's the same on main... |
|
This is the NIRISS example notebook on
|
| if instrument == "niriss": | ||
| pupil = fits.getheader(image_file, ext=0).get('PUPIL') | ||
| elif instrument == "nircam": | ||
| pupil = fits.getheader(image_file, ext=0).get('FILTER') |
There was a problem hiding this comment.
Should we add an else here or by the time you get here, it is either niriss or nircam and nothing else?
There was a problem hiding this comment.
Right now those are the only two possible values, nirspec uses a different parser and anything else throws an error in load_data.
There was a problem hiding this comment.
In that case, maybe less confusing to change
elif instrument == "nircam":to this?
else: # nircamThere was a problem hiding this comment.
I'm inclined to leave it as-is, since if we unify another instrument into this parser and forget to add a case for it, it will throw an error, as opposed to maybe quietly being handled incorrectly by the else statement.
There was a problem hiding this comment.
Then isn't it slightly prettier like this:
else: # pragma: no cover
raise NotImplementedError(f'{instrument} is not supported')
| @@ -190,8 +190,8 @@ def test_nircam_parser(mosviz_helper, tmp_path): | |||
| # Check that the correct amount of spectra got loaded in the correct order | |||
| assert len(mosviz_helper.app.data_collection) == 31 | |||
| assert mosviz_helper.app.data_collection['MOS Table']['Identifier'][0] == 1112 | |||
| assert mosviz_helper.app.data_collection[0].label == 'GRISMR Source 1112 spec2d R' | |||
| assert mosviz_helper.app.data_collection[15].label == 'GRISMR Source 1112 spec1d R' | |||
| assert mosviz_helper.app.data_collection[0].label == 'F322W2 Source 1112 spec2d R' | |||
There was a problem hiding this comment.
What does this mean? That the parser has been loading the wrong thing for NIRCam test case all this time? 😱
There was a problem hiding this comment.
It's loading the same thing, just labeling it differently. Due to differences in what goes in which header keyword between the instruments, NIRCam was trying to match up "GRISMR" from the 2D spectrum to "CLEAR" from the direct image, rather than this filter string like NIRISS does.
There was a problem hiding this comment.
@camipacifici probably needs to confirm that displaying this filter rather than "GRISMR" in the table is ok. Maybe we want both.
There was a problem hiding this comment.
Yes, it would be best to have both, but it can be part of a separate PR.
|
Great, thank you both. I'll open a followup issue about displaying both grism and filter information. |
I think this is all that needs to be done to allow for direct image loading, but I need @camipacifici to test on her data.