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.

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

Fix distortion reffile schema and unit test #5553

Merged
merged 3 commits into from
Jan 4, 2021

Conversation

jdavies-st
Copy link
Collaborator

@jdavies-st jdavies-st commented Dec 17, 2020

In debugging #5543 yesterday, warnings in the assign_wcs tests indicated that there was an inconsistency between the distortion model schema and what the validate() method for that model was requiring. validate() was requiring that meta.instrument.channel be populated, but the channel node was not actually in the schema. And this issue was being masked by the validate() code short-circuiting the real validation error message. So we were seeing the below:

$ pytest jwst/assign_wcs/tests/test_schemas.py 
=============================== test session starts ===============================
platform darwin -- Python 3.9.0, pytest-6.1.2, py-1.9.0, pluggy-0.13.1
rootdir: /Users/jdavies/dev/jwst, configfile: setup.cfg
plugins: asdf-2.7.1, requests-mock-1.8.0, doctestplus-0.8.0, ci-watson-0.5, cov-2.10.1, openfiles-0.5.0
collected 1 item                                                                  

jwst/assign_wcs/tests/test_schemas.py .                                     [100%]

================================ warnings summary =================================
jwst/assign_wcs/tests/test_schemas.py::test_distortion_schema
  /Users/jdavies/dev/jwst/jwst/datamodels/reference.py:45: ValidationWarning: Model.meta is missing values for ['description', 'author', 'pedigree', 'useafter']
    warnings.warn(message, ValidationWarning)

jwst/assign_wcs/tests/test_schemas.py::test_distortion_schema
  /Users/jdavies/dev/jwst/jwst/datamodels/wcs_ref_models.py:81: ValidationWarning: 
    warnings.warn(str(errmsg), ValidationWarning)

-- Docs: https://docs.pytest.org/en/stable/warnings.html
========================== 1 passed, 2 warnings in 0.71s ==========================

Once the masking of the real exception or warnings was fixed, it turns out the schema was not correct:

$ pytest jwst/assign_wcs/tests/test_schemas.py 
================================================== test session starts ==================================================
platform darwin -- Python 3.9.0, pytest-6.1.2, py-1.9.0, pluggy-0.13.1
rootdir: /Users/jdavies/dev/jwst, configfile: setup.cfg
plugins: asdf-2.7.1, requests-mock-1.8.0, doctestplus-0.8.0, ci-watson-0.5, cov-2.10.1, openfiles-0.5.0
collected 1 item                                                                                                        

jwst/assign_wcs/tests/test_schemas.py F                                                                           [100%]

======================================================= FAILURES ========================================================
________________________________________________ test_distortion_schema _________________________________________________

self = <stdatamodels.properties.ObjectNode object at 0x7fc550447c70>, attr = 'channel'

    def __getattr__(self, attr):
        from . import ndmodel
    
        if attr.startswith('_'):
            raise AttributeError('No attribute {0}'.format(attr))
    
        schema = _get_schema_for_property(self._schema, attr)
        try:
>           val = self._instance[attr]
E           KeyError: 'channel'

../../miniconda3/envs/jwst/lib/python3.9/site-packages/stdatamodels/properties.py:287: KeyError

During handling of the above exception, another exception occurred:

tmpdir = local('/private/var/folders/jg/by5st33j7ps356dgb4kn8w900001n5/T/pytest-of-jdavies/pytest-84/test_distortion_schema0')

    def test_distortion_schema(tmpdir):
        """Make sure DistortionModel roundtrips"""
        m = models.Shift(1) & models.Shift(2)
        dist = DistortionModel(model=m, input_units=u.pixel, output_units=u.arcsec)
    
        dist.meta.instrument.name = "NIRCAM"
        dist.meta.instrument.detector = "NRCA1"
        dist.meta.instrument.p_pupil = "F162M|F164N|CLEAR|"
        dist.meta.instrument.pupil = "F162M"
        dist.meta.exposure.p_exptype = "NRC_IMAGE|NRC_TSIMAGE|NRC_FLAT|NRC_LED|NRC_WFSC|"
        dist.meta.exposure.type = "NRC_IMAGE"
        dist.meta.psubarray = "FULL|SUB64P|SUB160)|SUB160P|SUB320|SUB400P|SUB640|"
        dist.meta.subarray.name = "FULL"
    
        dist.meta.instrument.module = "A"
        path = str(tmpdir.join("test_dist.asdf"))
>       dist.save(path)

jwst/assign_wcs/tests/test_schemas.py:22: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../miniconda3/envs/jwst/lib/python3.9/site-packages/stdatamodels/model_base.py:512: in save
    self.to_asdf(output_path, *args, **kwargs)
../../miniconda3/envs/jwst/lib/python3.9/site-packages/stdatamodels/model_base.py:578: in to_asdf
    self.validate()
jwst/datamodels/wcs_ref_models.py:77: in validate
    assert self.meta.instrument.channel is not None
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <stdatamodels.properties.ObjectNode object at 0x7fc550447c70>, attr = 'channel'

    def __getattr__(self, attr):
        from . import ndmodel
    
        if attr.startswith('_'):
            raise AttributeError('No attribute {0}'.format(attr))
    
        schema = _get_schema_for_property(self._schema, attr)
        try:
            val = self._instance[attr]
        except KeyError:
            if schema == {}:
>               raise AttributeError("No attribute '{0}'".format(attr))
E               AttributeError: No attribute 'channel'

../../miniconda3/envs/jwst/lib/python3.9/site-packages/stdatamodels/properties.py:290: AttributeError
=================================================== warnings summary ====================================================
jwst/assign_wcs/tests/test_schemas.py::test_distortion_schema
  /Users/jdavies/dev/jwst/jwst/datamodels/reference.py:45: ValidationWarning: Model.meta is missing values for ['description', 'author', 'pedigree', 'useafter']
    warnings.warn(message, ValidationWarning)

-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================================ short test summary info ================================================
FAILED jwst/assign_wcs/tests/test_schemas.py::test_distortion_schema - AttributeError: No attribute 'channel'
============================================= 1 failed, 1 warning in 0.79s ==============================================

Looking at existing reffiles for NIRCam and NIRISS, they actually have different metadata formats, and it's not clear there's consistency. Not sure I understand what is happening there.

But this PR at least makes the test and the schema in sync, and it also reports what the actual validation error/warning is now.

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #5553 (a3c8100) into master (98a36fc) will increase coverage by 0.06%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5553      +/-   ##
==========================================
+ Coverage   71.32%   71.39%   +0.06%     
==========================================
  Files         410      410              
  Lines       36467    36468       +1     
  Branches     5720     5753      +33     
==========================================
+ Hits        26010    26035      +25     
+ Misses       8800     8771      -29     
- Partials     1657     1662       +5     
Flag Coverage Δ
nightly 71.38% <80.00%> (+0.06%) ⬆️
unit 52.03% <80.00%> (+0.18%) ⬆️
Impacted Files Coverage Δ
jwst/datamodels/wcs_ref_models.py 64.90% <80.00%> (+13.03%) ⬆️
jwst/regtest/conftest.py 66.07% <0.00%> (-23.22%) ⬇️
jwst/regtest/regtestdata.py 75.89% <0.00%> (-2.24%) ⬇️
jwst/datamodels/reference.py 91.66% <0.00%> (+4.16%) ⬆️

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 98a36fc...a3c8100. Read the comment docs.

@hbushouse hbushouse added this to the Build 7.7 milestone Dec 17, 2020
@jdavies-st
Copy link
Collaborator Author

This branch passes the regression tests:

https://plwishmaster.stsci.edu:8081/job/RT/job/jdavies-dev/217/

And I also made sure that all the distortion reffiles currently in CRDS central store cache can be loaded:

In [1]: from jwst import datamodels

In [2]: from glob import glob

In [3]: files = glob("/grp/crds/jwst/references/jwst/jwst*distortion*.asdf")

In [4]: files.sort()

In [6]: for file in files:
   ...:     print("---------")
   ...:     with datamodels.DistortionModel(file) as model:
   ...:         print(file)
   ...:         model.validate()
   ...: 
---------
/grp/crds/jwst/references/jwst/jwst_fgs_distortion_0001.asdf
/Users/jdavies/dev/jwst/jwst/datamodels/reference.py:45: ValidationWarning: Model.meta is missing values for ['description', 'reftype', 'author', 'pedigree', 'useafter', 'instrument.name']
  warnings.warn(message, ValidationWarning)
/Users/jdavies/dev/jwst/jwst/datamodels/wcs_ref_models.py:61: ValidationWarning: Traceback (most recent call last):
  File "/Users/jdavies/dev/jwst/jwst/datamodels/wcs_ref_models.py", line 56, in validate
    assert self.meta.instrument.name in ["NIRCAM", "NIRSPEC", "MIRI", "TFI", "FGS", "NIRISS"]
AssertionError

  warnings.warn(traceback.format_exc(), ValidationWarning)
/Users/jdavies/dev/jwst/jwst/datamodels/wcs_ref_models.py:83: ValidationWarning: Traceback (most recent call last):
  File "/Users/jdavies/dev/jwst/jwst/datamodels/wcs_ref_models.py", line 73, in validate
    assert isinstance(self.meta.input_units, (str, u.NamedUnit))
AssertionError

  warnings.warn(traceback.format_exc(), ValidationWarning)
---------
/grp/crds/jwst/references/jwst/jwst_fgs_distortion_0002.asdf
---------
/grp/crds/jwst/references/jwst/jwst_fgs_distortion_0003.asdf
---------
/grp/crds/jwst/references/jwst/jwst_fgs_distortion_0004.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0001.asdf
/Users/jdavies/dev/jwst/jwst/datamodels/wcs_ref_models.py:61: ValidationWarning: Traceback (most recent call last):
  File "/Users/jdavies/dev/jwst/jwst/datamodels/wcs_ref_models.py", line 55, in validate
    assert isinstance(self.model, Model)
AssertionError

  warnings.warn(traceback.format_exc(), ValidationWarning)
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0002.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0003.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0004.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0005.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0006.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0007.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0008.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0009.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0010.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0011.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0012.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0013.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0014.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0015.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0016.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0017.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0018.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0019.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0020.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0021.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0022.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0023.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0024.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0025.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0026.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0027.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0028.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0029.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0030.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0031.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0032.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0033.asdf
---------
/grp/crds/jwst/references/jwst/jwst_miri_distortion_0034.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0001.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0002.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0003.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0004.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0005.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0006.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0007.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0008.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0009.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0010.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0011.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0012.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0013.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0014.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0015.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0016.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0017.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0018.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0019.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0020.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0021.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0022.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0023.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0024.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0025.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0026.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0027.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0028.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0029.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0030.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0033.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0034.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0035.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0036.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0037.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0038.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0039.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0040.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0041.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0042.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0043.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0044.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0045.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0046.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0047.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0048.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0049.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0050.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0051.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0052.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0053.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0054.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0055.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0056.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0057.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0058.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0059.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0060.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0061.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0062.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0063.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0064.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0065.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0066.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0085.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0086.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0087.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0088.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0089.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0090.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0091.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0092.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0093.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0094.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0095.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0096.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0097.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0098.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0099.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0100.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0101.asdf
---------
/grp/crds/jwst/references/jwst/jwst_nircam_distortion_0102.asdf
---------
/grp/crds/jwst/references/jwst/jwst_niriss_distortion_0001.asdf
---------
/grp/crds/jwst/references/jwst/jwst_niriss_distortion_0002.asdf
---------
/grp/crds/jwst/references/jwst/jwst_niriss_distortion_0003.asdf
---------
/grp/crds/jwst/references/jwst/jwst_niriss_distortion_0006.asdf
---------
/grp/crds/jwst/references/jwst/jwst_niriss_distortion_0008.asdf
---------
/grp/crds/jwst/references/jwst/jwst_niriss_distortion_0010.asdf

@jdavies-st
Copy link
Collaborator Author

jdavies-st commented Dec 20, 2020

The only odd thing here in the validate() method code is that problems in validation with these files can raise 3 different types of errors: ValidationError, KeyError and ValueError. Not sure if that was intentional or if it might be better to always raise a ValidationError?

Generally these validate() methods are carrying water for validation that is not done at the jsonschema.ValidationError level, either because we haven't written a validator to plug in to jsonschema for what we're trying to validate, or because jwst.datamodels breaks the standard JSON validation, i.e. in the case of required properties, due to the way it breaks up and validates tree fragments with schema fragments.

@jdavies-st jdavies-st requested a review from eslavich December 20, 2020 22:18
Copy link
Collaborator

@eslavich eslavich left a comment

Choose a reason for hiding this comment

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

This LGTM. I like the idea of changing stdatamodels to throw a consistent validation error class.

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 good to me. Not sure exactly why CHANNEL is needed in the ref files, because the DETECTOR specification already includes it indirectly for NIRCam (i.e. any DETECTOR with a trailing 1-4 is CHANNEL=SHORT, and any DETECTOR that ends with "LONG" is obviously CHANNEL=LONG), but since the keyword is in the ref files we should have it in the datamodel schema too.

@hbushouse
Copy link
Collaborator

Is this ready to be merged? If so, we can get it into a regtest run and potentially include it in the next B7.7 release candidate (0.18.1).

@jdavies-st
Copy link
Collaborator Author

Yeah, not sure why CHANNEL was an added selector, but it is.

Screen Shot 2021-01-04 at 11 32 55 AM

Let's merge.

@jdavies-st jdavies-st merged commit f34f518 into spacetelescope:master Jan 4, 2021
@jdavies-st jdavies-st deleted the distortion-schema branch January 4, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants