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

Jump to bottom

Add migrate_data script to make older x1d files readable by later versions of jwst #6055

Merged
merged 1 commit into from
May 28, 2021
Merged

Add migrate_data script to make older x1d files readable by later versions of jwst #6055

merged 1 commit into from
May 28, 2021

Conversation

eslavich
Copy link
Collaborator

@eslavich eslavich commented May 21, 2021 •
edited
Loading

Description

This PR adds a migrate_data script that will update the EXTRACT1D table to the new format introduced in #6014. It needs documentation and a test of some kind, but I'd like to get feedback first in case I need to make significant changes to the script.

Checklist

  • Tests

  • Documentation

  • Change log

  • Milestone

  • Label(s)

@eslavich eslavich marked this pull request as draft May 21, 2021 19:21
@eslavich eslavich requested a review from hbushouse May 21, 2021 19:22
@codecov
Copy link

codecov bot commented May 21, 2021 •
edited
Loading

Codecov Report

Merging #6055 (8d106e5) into master (22a204e) will increase coverage by 0.00%.
The diff coverage is 92.30%.

❗ Current head 8d106e5 differs from pull request most recent head 3e4f4fe. Consider uploading reports for the commit 3e4f4fe to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6055   +/-   ##
=======================================
  Coverage   77.13%   77.14%           
=======================================
  Files         402      402           
  Lines       34332    34357   +25     
=======================================
+ Hits        26483    26505   +22     
- Misses       7849     7852    +3
Flag Coverage Δ *Carryforward flag
nightly 77.13% <92.30%> (-0.01%) ⬇️ Carriedforward from 6d483a3
unit 55.17% <0.00%> (?)

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
jwst/regtest/regtestdata.py 84.75% <90.00%> (ø)
jwst/regtest/conftest.py 91.91% <100.00%> (+3.36%) ⬆️
pytest_crds/plugin.py 33.33% <0.00%> (-9.53%) ⬇️
jwst/white_light/white_light.py 91.30% <0.00%> (-2.18%) ⬇️
jwst/associations/asn_edit.py 66.66% <0.00%> (-0.75%) ⬇️
jwst/associations/lib/process_list.py 73.49% <0.00%> (-0.59%) ⬇️
jwst/combine_1d/combine1d.py 89.78% <0.00%> (-0.47%) ⬇️
jwst/fits_generator/template.py 16.98% <0.00%> (-0.27%) ⬇️
jwst/timeconversion/time_conversion.py 10.96% <0.00%> (-0.08%) ⬇️
jwst/associations/lib/rules_level2b.py 96.70% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22a204e...3e4f4fe. Read the comment docs.

hbushouse
hbushouse reviewed May 24, 2021
View reviewed changes
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Looks reasonable overall. At first I thought the name of the script was too generic, but then I saw that was intentional, because migrating the current x1d files is just one change allowed for. Future expansion: what a concept.

@eslavich eslavich force-pushed the eslavich-data-migration-script branch from 55b289a to 64894e9 Compare May 24, 2021 23:04
@eslavich eslavich requested a review from jdavies-st May 24, 2021 23:06
@eslavich eslavich marked this pull request as ready for review May 24, 2021 23:07
@eslavich eslavich requested a review from hbushouse May 25, 2021 13:20
@eslavich eslavich added this to the Build 7.8 milestone May 25, 2021
jdavies-st
jdavies-st reviewed May 25, 2021
View reviewed changes
jwst/regtest/test_migrate_data.py Outdated
@pytest.mark.parametrize("truth_path", [
"truth/test_miri_lrs_slit_spec2/jw00623032001_03102_00001_mirimage_x1d.fits",
"truth/test_nirspec_mos_spec2/f170lp-g235m_mos_observation-6-c0e0_001_dn_nrs1_mod_x1d.fits",
])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add an ids=["miri-lrs-x1d", "nirspec-mos-x1d"] to this parametrize call, so that the verbose output is readable. See

https://docs.pytest.org/en/6.2.x/example/parametrize.html#different-options-for-test-ids

jdavies-st
jdavies-st reviewed May 25, 2021
View reviewed changes
scripts/migrate_data Outdated
Comment on lines 55 to 64
def migrate_spec_model_1_1_0(hdul):
for hdu in hdul:
if hdu.name == "EXTRACT1D":
hdu.data = rebuild_spec_table(hdu.data)


def migrate_multispec_model_1_1_0(hdul):
for hdu in hdul:
if hdu.name == "EXTRACT1D":
hdu.data = rebuild_spec_table(hdu.data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These functions are identical. Is there a reason they are differently-named? Can't the _MIGRATE_METHODS dict below point to a single function for the two different datamodel classes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... good point, I thought I was going to need different logic for MultiSpecModel but they sure are identical.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made the exact same discovery when writing the readers for specutils for these a year ago. 😅

jdavies-st
jdavies-st approved these changes May 25, 2021
View reviewed changes
Copy link
Collaborator

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

This looks great. And the updates to RegtestData are a nice enhancement as well. 🚀

@eslavich eslavich force-pushed the eslavich-data-migration-script branch 2 times, most recently from a0fa600 to 159f414 Compare May 28, 2021 14:15
hbushouse
hbushouse approved these changes May 28, 2021
View reviewed changes
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

LGTM

@hbushouse
Copy link
Collaborator

Should be ready to merge, after fixing conflicts in change log.

@eslavich eslavich force-pushed the eslavich-data-migration-script branch from 159f414 to 3e4f4fe Compare May 28, 2021 18:11
@eslavich eslavich merged commit 5de9229 into spacetelescope:master May 28, 2021
@eslavich eslavich deleted the eslavich-data-migration-script branch May 28, 2021 18:49
@tapastro tapastro mentioned this pull request Jun 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbushouse hbushouse hbushouse approved these changes

@jdavies-st jdavies-st jdavies-st approved these changes

Assignees
No one assigned
Labels
scripts
Projects
None yet
Milestone
Build 7.8
Development

Successfully merging this pull request may close these issues.
3 participants
@eslavich @hbushouse @jdavies-st