Repository: spacetelescope/jwst

Issue:
datamodels should not serialize null values
_Issue [JP-1679](https://jira.stsci.edu/browse/JP-1679) was created on JIRA by [James Davies](https://jira.stsci.edu/secure/ViewProfile.jspa?name=jdavies):_

Currently, `datamodels` relies on undocumented behavior in `asdf` to strip elements in the tree that are set to `None` out of the tree before validating and writing the file out.

This behavior on the part of `asdf` is going away.  See

asdf-format/asdf#536
asdf-format/asdf#863

Using the above branch of `asdf`, this is a fairly typical error we get currently on `master`:


```
E           RuntimeError: Traceback (most recent call last):
E             File "/Users/jdavies/dev/jwst/jwst/pipeline/calwebb_spec2.py", line 112, in process
E               result = self.process_exposure_product(
E             File "/Users/jdavies/dev/jwst/jwst/pipeline/calwebb_spec2.py", line 240, in process_exposure_product
E               calibrated = self._process_nirspec_slits(calibrated)
E             File "/Users/jdavies/dev/jwst/jwst/pipeline/calwebb_spec2.py", line 376, in _process_nirspec_slits
E               calibrated = self.srctype(calibrated)
E             File "/Users/jdavies/dev/jwst/jwst/stpipe/step.py", line 453, in run
E               self.save_model(result, idx=idx)
E             File "/Users/jdavies/dev/jwst/jwst/stpipe/step.py", line 878, in save_model
E               output_path = model.save(
E             File "/Users/jdavies/dev/jwst/jwst/datamodels/model_base.py", line 476, in save
E               self.to_fits(output_path, *args, **kwargs)
E             File "/Users/jdavies/dev/jwst/jwst/datamodels/model_base.py", line 587, in to_fits
E               with fits_support.to_fits(self._instance, self._schema) as ff:
E             File "/Users/jdavies/dev/jwst/jwst/datamodels/fits_support.py", line 396, in to_fits
E               _save_from_schema(hdulist, tree, schema)
E             File "/Users/jdavies/dev/jwst/jwst/datamodels/fits_support.py", line 353, in _save_from_schema
E               validator.validate(tree, _schema=schema)
E             File "/Users/jdavies/miniconda3/envs/jwst/lib/python3.8/site-packages/jsonschema/validators.py", line 353, in validate
E               raise error
E           jsonschema.exceptions.ValidationError: None is not one of ['EXTENDED', 'POINT', 'UNKNOWN']
E           
E           Failed validating 'enum' in schema['properties']['meta']['properties']['target']['properties']['source_type_apt']:
E               OrderedDict([('title', 'Source type from APT (point/extended)'),
E                            ('type', 'string'),
E                            ('enum', ['EXTENDED', 'POINT', 'UNKNOWN']),
E                            ('fits_keyword', 'SRCTYAPT'),
E                            ('blend_table', True)])
E           
E           On instance['meta']['target']['source_type_apt']:
E               None
```



Solutions could be

- have `datamodels` itself strip attributes set to `None` out of the tree before validating and serializing, retaining that behavior in `datamodels` and not relying on `asdf` to do so.

- don't strip `None` values at all, and fix all the areas in `jwst` where we assign them, instead using `del <attribute>` instead.  This would work for us, but users might run into issues if they've been relying on this behavior in the past and have used it in their own code.

Related, simply accessing an attribute sets it to `None` and returns it.  So checking if something exists makes it exists and usually with an invalid `None` value that is not explicitly allowed by the schemas.  So this will probably have to change as well.

Perhaps don't create attributes on access?  Just return `None` to preserve the old behavior.  And create the attribute only on assignment.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/spacetelescope/jwst/pull/5371?src=pr&el=h1) Report
> Merging [#5371](https://codecov.io/gh/spacetelescope/jwst/pull/5371?src=pr&el=desc) into [master](https://codecov.io/gh/spacetelescope/jwst/commit/6e30e7ecf04a7abe40ba644ff6c5311b6a082cbd?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `85.71%`.

[![Impacted file tree graph](https://codecov.io/gh/spacetelescope/jwst/pull/5371/graphs/tree.svg?width=650&height=150&src=pr&token=Utf5Zs9g7z)](https://codecov.io/gh/spacetelescope/jwst/pull/5371?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #5371      +/-   ##
==========================================
- Coverage   52.88%   52.87%   -0.01%     
==========================================
  Files         409      409              
  Lines       37098    37100       +2     
  Branches     5760     5761       +1     
==========================================
- Hits        19618    19616       -2     
- Misses      16231    16233       +2     
- Partials     1249     1251       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| #unit | `52.87% <85.71%> (-0.01%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/spacetelescope/jwst/pull/5371?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [jwst/datamodels/container.py](https://codecov.io/gh/spacetelescope/jwst/pull/5371/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL2NvbnRhaW5lci5weQ==) | `74.31% <ø> (-0.55%)` | :arrow_down: |
| [jwst/resample/resample\_spec\_step.py](https://codecov.io/gh/spacetelescope/jwst/pull/5371/diff?src=pr&el=tree#diff-andzdC9yZXNhbXBsZS9yZXNhbXBsZV9zcGVjX3N0ZXAucHk=) | `75.00% <0.00%> (-2.22%)` | :arrow_down: |
| [jwst/datamodels/model\_base.py](https://codecov.io/gh/spacetelescope/jwst/pull/5371/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL21vZGVsX2Jhc2UucHk=) | `85.45% <100.00%> (+0.27%)` | :arrow_up: |
| [jwst/resample/resample\_spec.py](https://codecov.io/gh/spacetelescope/jwst/pull/5371/diff?src=pr&el=tree#diff-andzdC9yZXNhbXBsZS9yZXNhbXBsZV9zcGVjLnB5) | `82.75% <100.00%> (+0.08%)` | :arrow_up: |
| [jwst/datamodels/reference.py](https://codecov.io/gh/spacetelescope/jwst/pull/5371/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL3JlZmVyZW5jZS5weQ==) | `83.33% <0.00%> (-4.17%)` | :arrow_down: |
| [jwst/ramp\_fitting/utils.py](https://codecov.io/gh/spacetelescope/jwst/pull/5371/diff?src=pr&el=tree#diff-andzdC9yYW1wX2ZpdHRpbmcvdXRpbHMucHk=) | `95.25% <0.00%> (-0.08%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/spacetelescope/jwst/pull/5371?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/spacetelescope/jwst/pull/5371?src=pr&el=footer). Last update [6e30e7e...afc109b](https://codecov.io/gh/spacetelescope/jwst/pull/5371?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

hbushouse: Sorry, I updated the change log this morning for my own changes in another PR, so now you've got conflicts.
jdavies-st: No problem Howard.  I've added a simple unit test to this PR as well, to show this works, though clearly this is being tested elsewhere implicitly as well.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index c5e485f53..5e41dfd64 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -10,6 +10,11 @@ cube_build
 - If outlier detection has flagged all the data on a input file as DO_NOT_USE, then
   skip the file in creating an ifucube [*5347]
 
+datamodels
+----------
+
+- Skip serializing `None` in datamodels to be compatible with `asdf>=2.8` [#5371]
+
 extract_1d
 ----------
 
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 1a4bf3570..effe1d086 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -1,8 +1,7 @@
 Please open a new issue or new pull request for bugs, feedback, or new features you would like to see. If there is an issue you would like to work on, please leave a comment and we will be happy to assist. New contributions and contributors are very welcome!
 
-The main development work is done on the "master" branch. The "stable" branch is protected and used for official releases. The rest of the branches are for release maintenance and should not be used normally. Unless otherwise told by a maintainer, pull request should be made and submitted to the "master" branch.
+The main development work is done on the "master" branch. The "stable" branch is protected and used for official releases. The rest of the branches are for release maintenance and should not be used normally. Unless otherwise told by a maintainer, pull requests should be made and submitted to the "master" branch.
 
-New to github or open source projects? If you are unsure about where to start or haven't used github before, please feel free to contact the package maintainers. 
-
-Feedback and feature requests? Is there something missing you would like to see? Please open an issue or send an email to the maintainers. This package follows the Spacetelescope `Code of Conduct<CODE_OF_CONDUCT.md>`_ strives to provide a welcoming community to all of our users and contributors.
+New to GitHub or open source projects? If you are unsure about where to start or haven't used GitHub before, please feel free to contact the package maintainers.
 
+Feedback and feature requests? Is there something missing you would like to see? Please open an issue or send an email to the maintainers. This package follows the Spacetelescope [Code of Conduct](CODE_OF_CONDUCT.md) strives to provide a welcoming community to all of our users and contributors.
diff --git a/jwst/datamodels/container.py b/jwst/datamodels/container.py
index a9fa9be97..e4b599fae 100644
--- a/jwst/datamodels/container.py
+++ b/jwst/datamodels/container.py
@@ -1,7 +1,6 @@
 import copy
 from collections import OrderedDict
 import os.path as op
-import warnings
 import re
 import logging
 
@@ -348,12 +347,6 @@ class ModelContainer(model_base.DataModel):
                                              ''.join(params[3:6]), params[6]]))
                 model.meta.group_id = group_id
             except TypeError:
-                params_dict = dict(zip(unique_exposure_parameters, params))
-                bad_params = {'meta.observation.'+k:v for k, v in params_dict.items() if not v}
-                warnings.warn(
-                    'Cannot determine grouping of exposures: '
-                    '{}'.format(bad_params)
-                    )
                 model.meta.group_id = 'exposure{0:04d}'.format(i + 1)
 
     @property
diff --git a/jwst/datamodels/model_base.py b/jwst/datamodels/model_base.py
index 9b649940d..75c9bdeea 100644
--- a/jwst/datamodels/model_base.py
+++ b/jwst/datamodels/model_base.py
@@ -17,9 +17,16 @@ from astropy.wcs import WCS
 
 import asdf
 from asdf import AsdfFile
-from asdf import yamlutil
+from asdf import yamlutil, treeutil
 from asdf import schema as asdf_schema
 
+try:
+    from asdf.treeutil import RemoveNode
+except ImportError:
+    # Prior to asdf 2.8, None was used to indicate
+    # that a node should be removed.
+    RemoveNode = None
+
 from . import ndmodel
 from . import filetype
 from . import fits_support
@@ -439,6 +446,15 @@ class DataModel(properties.ObjectNode, ndmodel.NDModel):
         # Enforce model_type to be the actual type of model being saved.
         self.meta.model_type = self._model_type
 
+        # Remove None nodes from the tree:
+        def _remove_none(node):
+            if node is None:
+                return RemoveNode
+            else:
+                return node
+
+        self._instance = treeutil.walk_and_modify(self._instance, _remove_none)
+
     def save(self, path, dir_path=None, *args, **kwargs):
         """
         Save to either a FITS or ASDF file, depending on the path.
diff --git a/jwst/resample/resample_spec.py b/jwst/resample/resample_spec.py
index ac2bf383b..0beef0fb1 100644
--- a/jwst/resample/resample_spec.py
+++ b/jwst/resample/resample_spec.py
@@ -213,7 +213,8 @@ class ResampleSpecData:
             wavelength_array = np.flip(wavelength_array,axis=None)
 
         pix_to_wavelength = Tabular1D(lookup_table=wavelength_array,
-                                      bounds_error=False, fill_value=None, name='pix2wavelength')
+                                      bounds_error=False, fill_value=None,
+                                      name='pix2wavelength')
 
         # Tabular models need an inverse explicitly defined.
         # If the wavelength array is decending instead of ascending, both
@@ -227,7 +228,8 @@ class ResampleSpecData:
             lookup_table = lookup_table[::-1]
         pix_to_wavelength.inverse = Tabular1D(points=points,
                                               lookup_table=lookup_table,
-                                              bounds_error=False, fill_value=None, name='wavelength2pix')
+                                              bounds_error=False, fill_value=None,
+                                              name='wavelength2pix')
 
         # For the input mapping, duplicate the spatial coordinate
         mapping = Mapping((spatial_axis, spatial_axis, spectral_axis))
@@ -390,7 +392,9 @@ class ResampleSpecData:
             output_model.meta.resample.weight_type = self.drizpars['weight_type']
             output_model.meta.resample.pointings = pointings
 
-            # Update mutlislit slit info on the output_model
+            # Update slit info on the output_model. This is needed
+            # because model.slit attributes are not in model.meta, so the
+            # normal update() method doesn't work with them.
             for attr in ['name', 'xstart', 'xsize', 'ystart', 'ysize',
                          'slitlet_id', 'source_id', 'source_name', 'source_alias',
                          'stellarity', 'source_type', 'source_xpos', 'source_ypos',
@@ -400,7 +404,8 @@ class ResampleSpecData:
                 except AttributeError:
                     pass
                 else:
-                    setattr(output_model, attr, val)
+                    if val is not None:
+                        setattr(output_model, attr, val)
 
             self.output_models.append(output_model)
 
diff --git a/jwst/resample/resample_spec_step.py b/jwst/resample/resample_spec_step.py
index 983e04559..7f471bc47 100755
--- a/jwst/resample/resample_spec_step.py
+++ b/jwst/resample/resample_spec_step.py
@@ -110,7 +110,8 @@ class ResampleSpecStep(ResampleStep):
             # Everything resampled to single output model
             if len(drizzled_models) == 1:
                 result.slits.append(drizzled_models[0])
-                result.slits[-1].meta.bunit_data = container[0].meta.bunit_data
+                if container[0].meta.bunit_data is not None:
+                    result.slits[-1].meta.bunit_data = container[0].meta.bunit_data
             else:
                 # When each input is resampled to its own output
                 for model in drizzled_models:


Test patch:
diff --git a/jwst/datamodels/tests/test_models.py b/jwst/datamodels/tests/test_models.py
index 6e9f283c7..260e02ab5 100644
--- a/jwst/datamodels/tests/test_models.py
+++ b/jwst/datamodels/tests/test_models.py
@@ -891,3 +891,17 @@ def test_getarray_noinit_noinit():
     except AttributeError:
         pass
     assert 'area' not in model.instance
+
+
+@pytest.mark.parametrize("filename", ["null.fits", "null.asdf"])
+def test_skip_serializing_null(tmpdir, filename):
+    """Make sure that None is not written out to the ASDF tree"""
+    path = str(tmpdir.join(filename))
+    with DataModel() as model:
+        model.meta.telescope = None
+        model.save(path)
+
+    with DataModel(path) as model:
+        # Make sure that 'telescope' is not in the tree
+        with pytest.raises(KeyError):
+            assert model.meta["telescope"] is None
diff --git a/jwst/dq_init/tests/test_dq_init.py b/jwst/dq_init/tests/test_dq_init.py
index d175a426b..7bdd5feef 100644
--- a/jwst/dq_init/tests/test_dq_init.py
+++ b/jwst/dq_init/tests/test_dq_init.py
@@ -194,9 +194,11 @@ def test_dq_subarray():
     ref_data.meta.subarray.xsize = fullxsize
     ref_data.meta.subarray.ystart = 1
     ref_data.meta.subarray.ysize = fullysize
-
-    # Filter out validation warnings from ref_data
-    warnings.filterwarnings("ignore", category=ValidationWarning)
+    ref_data.meta.description = "foo"
+    ref_data.meta.reftype = "mask"
+    ref_data.meta.author = "pytest"
+    ref_data.meta.pedigree = "foo"
+    ref_data.meta.useafter = "2000-01-01T00:00:00"
 
     # run correction step
     outfile = do_dqinit(im, ref_data)