Skip to content
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.

Sign up for GitHub

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 Direct Image to expected file list for NIRCam #1948

Merged
merged 4 commits into from
Jan 3, 2023
Merged

Add Direct Image to expected file list for NIRCam #1948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fae3a3b
Add Direct Image to expected file list for NIRCam
rosteen Dec 23, 2022
4dd2521
Fix filter retrieval for NIRCam
rosteen Dec 30, 2022
1311ac4
Update changelog
rosteen Dec 30, 2022
3955bcd
Update test, also get matching filter in 1D spectra
rosteen Dec 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ Mosviz

- ``mosviz_row`` metadata now included in NIRISS-parsed 1D spectra. [#1836]

- Now loads NIRCam direct image properly when loading a directory. [#1948]

Specviz
^^^^^^^

Expand Down
4 changes: 3 additions & 1 deletion jdaviz/configs/mosviz/helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,13 +433,15 @@ def load_data(self, spectra_1d=None, spectra_2d=None, images=None,

directory = kwargs.pop('directory', None)
instrument = kwargs.pop('instrument', None)
if instrument is not None:
instrument = instrument.lower()

if directory is not None and Path(directory).is_dir():
if instrument not in ('nirspec', 'niriss', 'nircam'):
raise ValueError(
"Ambiguous MOS Instrument: Only JWST NIRSpec, NIRCam, and "
f"NIRISS folder parsing are currently supported but got '{instrument}'")
if instrument.lower() == "nirspec":
if instrument == "nirspec":
super().load_data(directory, parser_reference="mosviz-nirspec-directory-parser")
else: # niriss or nircam
self.load_jwst_directory(directory, instrument=instrument)
Expand Down
20 changes: 15 additions & 5 deletions jdaviz/configs/mosviz/plugins/parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
'2D Spectra C', '2D Spectra R',
'Direct Image'],
"nircam": ['1D Spectra C', '1D Spectra R',
'2D Spectra C', '2D Spectra R'],
'2D Spectra C', '2D Spectra R',
'Direct Image'],
"nirspec": ['1D Spectra', '2D Spectra']}


Expand Down Expand Up @@ -738,7 +739,10 @@ def mos_niriss_parser(app, data_dir, instrument=None,
for image_file in files_by_labels["Direct Image"]:
# save label for table viewer
im_split = image_file.stem.split("_")[0]
pupil = fits.getheader(image_file, ext=0).get('PUPIL')
if instrument == "niriss":
pupil = fits.getheader(image_file, ext=0).get('PUPIL')
elif instrument == "nircam":
pupil = fits.getheader(image_file, ext=0).get('FILTER')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add an else here or by the time you get here, it is either niriss or nircam and nothing else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now those are the only two possible values, nirspec uses a different parser and anything else throws an error in load_data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, maybe less confusing to change

elif instrument == "nircam":

to this?

else:  # nircam

Copy link
Collaborator Author

@rosteen rosteen Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then isn't it slightly prettier like this:

else:  # pragma: no cover
    raise NotImplementedError(f'{instrument} is not supported')


image_label = f"Image {im_split} {pupil}"
image_dict[pupil] = image_label
Expand Down Expand Up @@ -770,7 +774,10 @@ def mos_niriss_parser(app, data_dir, instrument=None,
for fname in files_by_labels[flabel]:
print(f"Loading: {flabel} sources")
if flabel in ('2D Spectra R', '2D Spectra C'):
filter_name = fits.getheader(fname, ext=0).get('PUPIL')
if instrument == "niriss":
filter_name = fits.getheader(fname, ext=0).get('PUPIL')
elif instrument == "nircam":
filter_name = fits.getheader(fname, ext=0).get('FILTER')

# Orientation denoted by "C", "R", or "C+R" for combined spectra
orientation = flabel.split()[-1]
Expand Down Expand Up @@ -812,7 +819,7 @@ def mos_niriss_parser(app, data_dir, instrument=None,
wav = temp[wav_hdus[sci]].data.mean(axis=0) * u.micron

spec2d = Spectrum1D(data * u.one, spectral_axis=wav, meta=meta)
spec2d.meta['INSTRUME'] = 'NIRISS'
spec2d.meta['INSTRUME'] = instrument.upper()
spec2d.meta['mosviz_row'] = len(spec_labels_2d)

label = (f"{filter_name} Source "
Expand Down Expand Up @@ -862,7 +869,10 @@ def mos_niriss_parser(app, data_dir, instrument=None,
raise KeyError(f"The SRCTYPE keyword in the header of file {fname} "
"is not populated (expected values: EXTENDED or POINT)") from e

filter_name = fits.getheader(fname, ext=0).get('PUPIL')
if instrument == "niriss":
filter_name = fits.getheader(fname, ext=0).get('PUPIL')
elif instrument == "nircam":
filter_name = fits.getheader(fname, ext=0).get('FILTER')

# Orientation denoted by "C", "R", or "C+R" for combined spectra
orientation = flabel.split()[-1]
Expand Down
4 changes: 2 additions & 2 deletions jdaviz/configs/mosviz/tests/test_parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean? That the parser has been loading the wrong thing for NIRCam test case all this time? 😱

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camipacifici probably needs to confirm that displaying this filter rather than "GRISMR" in the table is ok. Maybe we want both.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be best to have both, but it can be part of a separate PR.

assert mosviz_helper.app.data_collection[15].label == 'F322W2 Source 1112 spec1d R'


@pytest.mark.remote_data
Expand Down