extract1d for IFU data version 2- JP 1736 #5506

jemorrison · 2020-12-02T21:48:03Z

This is just a copy of #5383. Somehow that PR got confused on my system and I could not resolve conflicts.
This is just a clean copy.

Fixes JP-1615
Fixes JP-1633
Fixes JP-1730

typo fix

…rrison/jwst into jemorrison-extract1d_datamodel_JP_1736_v2 Problem with branch conflicts

codecov · 2020-12-02T22:15:36Z

Codecov Report

Merging #5506 (c55ca7c) into master (53dc5e2) will decrease coverage by 3.19%.
The diff coverage is 64.67%.

@@            Coverage Diff             @@
##           master    #5506      +/-   ##
==========================================
- Coverage   74.41%   71.21%   -3.20%     
==========================================
  Files         417      415       -2     
  Lines       38115    41147    +3032     
  Branches     5636     5891     +255     
==========================================
+ Hits        28362    29303     +941     
- Misses       9753    11001    +1248     
- Partials        0      843     +843

Flag	Coverage Δ		*Carryforward flag
nightly	`71.10% <84.73%> (-4.85%)`	⬇️	Carriedforward from 5712155
unit	`54.20% <16.66%> (-0.06%)`	⬇️

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

Impacted Files	Coverage Δ
jwst/extract_1d/apply_apcorr.py	`77.77% <50.00%> (-17.99%)`	⬇️
jwst/extract_1d/extract.py	`43.85% <53.84%> (-24.43%)`	⬇️
jwst/extract_1d/ifu.py	`63.17% <70.70%> (-16.65%)`	⬇️
jwst/extract_1d/extract_1d_step.py	`49.60% <87.50%> (-31.93%)`	⬇️
jwst/datamodels/__init__.py	`100.00% <100.00%> (ø)`
jwst/datamodels/apcorr.py	`100.00% <100.00%> (ø)`
jwst/datamodels/extract1d_spec.py	`100.00% <100.00%> (ø)`
jwst/reset/reset_step.py	`25.92% <0.00%> (-40.75%)`	⬇️
jwst/pipeline/calwebb_ami3.py	`61.90% <0.00%> (-32.00%)`	⬇️
jwst/reset/reset_sub.py	`70.00% <0.00%> (-30.00%)`	⬇️
... and 129 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 53dc5e2...c55ca7c. Read the comment docs.

drlaw1558 · 2020-12-02T22:36:21Z

@jemorrison This is meant to be a copy of PR-5383, but it looks like they each have different numbers of files changed? (14 here, 23 there) E.g., PR-5383 modified CHANGES.rst but this PR-5506 does not?

Trying to figure out the best way to compare the relevant code between these two PRs, and if it's best to just check out this new PR and rerun my various tests again.

jemorrison · 2020-12-02T22:39:46Z

________________________________ From: David Law <[email protected]> Sent: Wednesday, December 2, 2020 3:36 PM To: spacetelescope/jwst <[email protected]> Cc: Jane Morrison <[email protected]>; Mention <[email protected]> Subject: [EXT]Re: [spacetelescope/jwst] extract1d for IFU data version 2- JP 1736 (#5506) External Email External Email @jemorrison<https://github.com/jemorrison> This is meant to be a copy of PR-5383, but it looks like they each have different numbers of files changed? (14 here, 23 there) E.g., PR-5383 modified CHANGES.rst but this PR-5506 does not? Trying to figure out the best way to compare the relevant code between these two PRs, and if it's best to just check out this new PR and rerun my various tests again. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#5506 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AENCOWOEOCB44PGEBD7IP7TSS26PHANCNFSM4ULCZA7Q>.

jemorrison · 2020-12-02T22:42:09Z

Yes I just do not understand why the old PR had files changes related to associations. I did not touch code related to associations. I must of created the branch in a non-standard way. I just wanted the PR to include the extract1d changes. I think this is a clean pr how. I need to update the change log and I will do that and push that change over.
To create this new branch I did this:
git fetch jwst
git checkout -b jemorrison-extract1d_datamodel_JP_1736_v2 jwst/master
git pull https://github.com/jemorrison/jwst.git extract1d_datamodel_JP_1736
It only grabbed the extract1d code not the association related code.

drlaw1558 · 2020-12-02T22:44:06Z

Ok, I think I see now. I will see if it's possible to check everything just by comparing the line-by-line diffs for the non-association related files rather than by rerunning various simulations/tests.

drlaw1558 · 2020-12-02T22:55:30Z

What are all of the warnings (e.g., Added lines #L399 - L400 were not covered by tests) in apply_apcorr.py in the diff file about? They make it hard to scan the actual diffs as they break up the page so much.

jemorrison · 2020-12-03T14:21:15Z

@drlaw1558 I think the warnings are that I have added code but I do not have a unit test that tests these new lines.

hbushouse

Wow, this represents a LOT of work! Just a few comments and questions here and there. Can't really properly review the actual new functionality for IFU data, since I'm not completely sure how it's supposed to work.

CHANGES.rst

hbushouse · 2020-12-03T15:29:49Z

jwst/datamodels/__init__.py

@@ -21,6 +21,7 @@
 from .drizpars import DrizParsModel
 from .drizproduct import DrizProductModel
 from .extract1dimage import Extract1dImageModel
+from .extract1d_spec import IFUExtract1dModel


I suppose it would throw a big wrench into things at this late stage to rename this new model class to Extract1dIFUModel just for consistency in naming with the existing Extract1dImageModel? If so, it's not a big deal.

That is fine I can change the names of models to be more consistent

Ok, I think that means I'll have to change some things about my generation code as well. Are the 'datamodl' keywords still meant to be MirMrsExtract1dModel and MirMrsApcorrModel ? If so I should only need to change the names of the schema files.

Yes @drlaw1558 that does imply that the actual class name of the data model used for the extract1d reference file will be changing from IFUExtract1dModel to Extract1dIFUModel. That's only for the extract1d reference file model. The apcorr ref files models remain unchanged. I don't see MirMrsExtract1dModel referenced anywhere, or am I just missing it?

Hm, it looks like this was the name Jane used in creating a test asdf file back in October; maybe that's now been superseded by MirMrsExtract1dModel -> IFUExtract1dModel -> Extract1dIFUModel. In this case I'll need to change both my code and the reference file to make sure the data model keyword is up to date.

It doesn't look like the update has been made in the schema for this branch yet though? @jemorrison your comment above said that it had been changed, but the relevant branch still seems to have the old names?

I just pushed over the new code. It is Extract1DIFUModel now.

Ok, I see it. If we're now using schemas/extract1difu.schema.yaml should we remove the old schemas/mirmrs_extract1d.schema.yaml ?

hbushouse · 2020-12-03T15:36:23Z

jwst/datamodels/schemas/ifuextract1d.schema.yaml

@@ -0,0 +1,48 @@
+%YAML 1.1


Again in terms of file naming consistency, could this new schema file be renamed to extract1difu.schema.yaml, in keeping with the existing extract1dimage.schema.yaml file?

hbushouse · 2020-12-03T15:41:35Z

jwst/datamodels/schemas/mirmrs_apcorr.schema.yaml

+            type: string
+          wavelength:
+            datatype: float32


Why are some data type attributes named type while others are datatype? Does it make a difference? My inner hob goblin of foolish consistency suggests having them all the same.

It was trial and error to get this to work. This worked that is all I know

So from trial and error I have found that when I create the asdf reference files, write them to disk and then read them in
using asdf.open and test them using asdf validate - all values that are strings that are called datatype in the schema fail in the validate. The error message says they are not arrays. So I am guessing that 'datatype' is for arrays and 'type' is for single values.

hbushouse · 2020-12-03T15:44:39Z

jwst/datamodels/schemas/nrsifu_apcorr.schema.yaml

+            datatype: float32
+          apcorr_err:
+            datatype: float32
+    apcorr_table_f070lp_g140h:


I know it'd be a huge pain to do at this late stage of the development, but having cal data for all the filter+grating combinations in a single ref file forces the pipeline to do the selection of the proper data, while having them split into separate ref files would put the selection in CRDS, which is what it was built for.

So at one point we thought about pulling out filter/grating and having a different reference file for each. We ended up just leaving it in the code mainly because I was not sure how CRDS would handle the cases of multiple bands, channels, gratings or filters. After testing I think 'MULTIPLE' is set if that is the case - so we can go ahead and break then up into separate reference files. NOTHING has been loaded into CRDS.

hbushouse · 2020-12-03T15:51:16Z

jwst/extract_1d/apply_apcorr.py


+    def apply(self, spec_table: fits.FITS_rec):


Now that the apcorr ref files have been migrated to ASDF format, does the fits.FITS_rec still work properly? Or is the spec_table object the spectral data that've already been extracted and are still stored as a FITS table?

the spec_table is still stored as a FITS table.

hbushouse · 2020-12-03T16:06:54Z

jwst/extract_1d/extract.py

@@ -115,13 +116,23 @@ def open_extract1d_ref(refname: str) -> dict:
        If the extract1d reference file is in JSON format, ref_dict will be the
        dictionary returned by json.load(), except that the file type
        ('JSON') will also be included with key 'ref_file_type'.
+        If the extract1d reference file is in asdf format, the ref_dict will
+        be a containing two keys: ref_dict['ref_file_type'] = 'ASDF'


looks like a typo here: "will be a containing two ..." will be a what?

hbushouse · 2020-12-03T16:08:22Z

jwst/extract_1d/extract.py

    if refname == "N/A":
        ref_dict = None
+    elif exptype == 'MIR_MRS' or exptype == 'NRS_IFU':


Doesn't affect functionality, but you could condense this to:
elif exptype in ['MIR_MRS', 'NRS_IFU']:

hbushouse · 2020-12-03T16:14:37Z

jwst/extract_1d/tests/test_apply_apcorr_nonifu.py

@@ -5,12 +5,12 @@
 from astropy.io import fits
 from astropy.table import Table

-from jwst.datamodels import JwstDataModel
-from jwst.extract_1d.apply_apcorr import ApCorr, ApCorrRadial, ApCorrPhase, select_apcorr
+from jwst.datamodels import DataModel


Hmmm, this might need to be JwstDataModel, because I think DataModel went away with the migration to using the new stdatamodels package. Need to confirm.

hbushouse · 2020-12-03T16:15:41Z

jwst/extract_1d/tests/test_apply_apcorr_nonifu.py

@@ -35,7 +33,7 @@ def inputs(request):

    instrument, exptype = request.param

-    dm = JwstDataModel()
+    dm = DataModel()


Same comment here about JwstDataModel

…ne per grating/band

drlaw1558

Looks good to me. I recreated new reference files using the updated schema names, and reran some of my tests just to make sure.

hbushouse · 2021-01-07T15:57:48Z

flake8 issues need fixing before we merge

Fix flake8 whitespace issues.

jemorrison and others added 30 commits November 9, 2020 14:21

first attempt at setting up extract1d miri ref file for IFU data

09f3f79

remove shape from mirmrs_extract1d datamodel

3f125dc

fixed mirmrs_extract 1d data model

5ed9130

using new apcorr ref file

3db8ead

fix radius scaling

44057bc

update to clean up comments

a206a6c

fixed convert size units

1440140

using new apcorr ref file

b7064d5

update description of mir mrs apcorr ref file

0cc84aa

Updates based on review

1bc715a

flake8 fixes and comment clean up

5eecb27

more flake8 fixes

d9019f5

new mirmrs_extract1d for asdf ref file

290c14e

cleaned up ifuextract1d datamodel

49a2540

updates

aaf1e28

updates for using apcor ref asdf file

29a5504

flake8 fixes

50d4948

fix NrsIfuApcorrModel

7336a0e

updates to tests

7daf396

updates to tests

bc3b291

fix conflicts

473a65b

flake 8 datamodel extract1d_spec.py

107eb93

more flake fixes

8730676

fix tests

054a850

more flake8 fixes

fa30cb4

update to test_apply_apcorr_ifu.py

f611712

typo fix

c4e82b2

Merge pull request #4 from drlaw1558/drlnov12

dffe53d

typo fix

first attempt at setting up extract1d miri ref file for IFU data

066ca3f

remove shape from mirmrs_extract1d datamodel

182723f

Merge branch 'extract1d_datamodel_JP_1736' of https://github.com/jemo…

30c62db

…rrison/jwst into jemorrison-extract1d_datamodel_JP_1736_v2 Problem with branch conflicts

jemorrison changed the title ~~Jemorrison extract1d datamodel jp 1736 v2~~ extract1d for IFU data version 2- JP 1736 Dec 2, 2020

jemorrison requested review from hbushouse, jdavies-st and drlaw1558 December 2, 2020 22:28

jemorrison mentioned this pull request Dec 2, 2020

extract1d updates for IFU data #5383

Closed

jemorrison added 2 commits December 3, 2020 07:32

updated change log

b04466b

cleaned up comment

06e800f

hbushouse added the extract_1d label Dec 3, 2020

hbushouse added this to the Build 7.7 milestone Dec 3, 2020

hbushouse reviewed Dec 3, 2020

View reviewed changes

jemorrison added 2 commits December 4, 2020 12:46

update from review. Changed how apcor ref file is done for NIRSpec. O…

b3bcc1c

…ne per grating/band

fix typo

196b89a

drlaw1558 approved these changes Dec 4, 2020

View reviewed changes

stscijgbot-jp mentioned this pull request Dec 4, 2020

IFU changes to Extract1D #5361

Closed

Merge branch 'master' into jemorrison-extract1d_datamodel_JP_1736_v2

5712155

Update extract_1d_step.py

c55ca7c

Fix flake8 whitespace issues.

hbushouse approved these changes Jan 7, 2021

View reviewed changes

hbushouse merged commit 31a1c5f into spacetelescope:master Jan 7, 2021

This was referenced Jan 12, 2021

Bug in MRS extract_1d and aperture correction #5190

Closed

Incorrect application of ApCorrRadial interpolation function #5227

Closed

jemorrison deleted the jemorrison-extract1d_datamodel_JP_1736_v2 branch February 24, 2021 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extract1d for IFU data version 2- JP 1736 #5506

extract1d for IFU data version 2- JP 1736 #5506

jemorrison commented Dec 2, 2020 •

edited by hbushouse

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading

drlaw1558 commented Dec 2, 2020

jemorrison commented Dec 2, 2020 via email

jemorrison commented Dec 2, 2020 •

edited

Loading

drlaw1558 commented Dec 2, 2020

drlaw1558 commented Dec 2, 2020

jemorrison commented Dec 3, 2020

hbushouse left a comment

hbushouse Dec 3, 2020

jemorrison Dec 3, 2020

jemorrison Dec 3, 2020

drlaw1558 Dec 3, 2020

hbushouse Dec 4, 2020

drlaw1558 Dec 4, 2020

jemorrison Dec 4, 2020

drlaw1558 Dec 4, 2020

hbushouse Dec 3, 2020

jemorrison Dec 3, 2020

hbushouse Dec 3, 2020

jemorrison Dec 3, 2020

jemorrison Dec 4, 2020

hbushouse Dec 3, 2020

jemorrison Dec 3, 2020

hbushouse Dec 3, 2020

jemorrison Dec 3, 2020

hbushouse Dec 3, 2020

hbushouse Dec 3, 2020

hbushouse Dec 3, 2020

hbushouse Dec 3, 2020

drlaw1558 left a comment

hbushouse commented Jan 7, 2021

extract1d for IFU data version 2- JP 1736 #5506

extract1d for IFU data version 2- JP 1736 #5506

Conversation

jemorrison commented Dec 2, 2020 • edited by hbushouse Loading

codecov bot commented Dec 2, 2020 • edited Loading

Codecov Report

drlaw1558 commented Dec 2, 2020

jemorrison commented Dec 2, 2020 via email

jemorrison commented Dec 2, 2020 • edited Loading

drlaw1558 commented Dec 2, 2020

drlaw1558 commented Dec 2, 2020

jemorrison commented Dec 3, 2020

hbushouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drlaw1558 left a comment

Choose a reason for hiding this comment

hbushouse commented Jan 7, 2021

jemorrison commented Dec 2, 2020 •

edited by hbushouse

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading

jemorrison commented Dec 2, 2020 •

edited

Loading