Repository: spacetelescope/jwst

Issue:
Add hash checking of the FITS component to DataModel I/O
_Issue [JP-1457](https://jira.stsci.edu/browse/JP-1457) was created on JIRA by [Jonathan Eisenhamer](https://jira.stsci.edu/secure/ViewProfile.jspa?name=eisenham):_

An optimization that can greatly speed up the reading of FITS files as `DataModels` is to skip the FITS verification/loading routine\` load_from_schema`. This is the routine that reads the FITS headers, according to the provided schemas, and loads ASDF tree. This is the slowest part of the I/O since it requires walking the schema, often over arrays of schemas.

This routine is necessary when either the FITS file does not have the ASDF component already, or to ensure that any changes to FITS keywords are incorporated into the ASDF component.

When a `DataModel` is written to file, the ASDF component is added to the file and contains all the information that is also in the FITS headers. Hence, on read, if it can be determined that the FITS component had not changed, the ASDF component can simply be read and the `load_from_schema` routine skipped.

This can be checked programmatically when, on write, a hash of the FITS headers is calculated and added to the ASDF component. Then, on read, the FITS hash is re-calculated. If it is the same as the stored hash, then the routine can safely be skipped.

In resolving JP-1343, this approach was experimented with and a few issues where found, none of which were show-stopping in nature.

The first is the data components. Data arrays are kept in a FITS HDU and not duplicated in the ASDF tree, conserving memory. This is implemented in ASDF itself using a lazy-load method. A side effect of `load_from_schema` is to trigger the loading of these data segments. Skipping this is fine; the lazy loading kicks in when the data are actually referred to later on. However, this requires the FITS file to remain open during the life of the `DataModel`. Again, this is fine except there is inconsistency in `DataModes` code itself; sometimes the FITS file is left open, sometimes not. An initial PR #&#x2060;4930, resolves this issue. However, this does cascade into how DataModels are further copied, and closed, in application code. Some examination of this will need be done. Or the option is to just do the data-schema loading regardless. This component of the load is quite quicker than the FITS keywords, only because there are not that many of them compared to the number of keywords to load.

Another thing that `load_from_schema` does is determine which HDUs are not part of the schema at all, placing them in the `extra_fits` part of the ASDF tree. A way of determining extra_fits will need be done without having to incur the full cost of the loading.

Discussion:
stscieisenhamer: Merge only after PR #4930 is merged.
stscieisenhamer: WRT the `bit_flags_to_mnemonics` function: Have done the suggested rename and added an argument for the different mnemonic mappings.
codecov[bot]: # [Codecov](https://codecov.io/gh/spacetelescope/jwst/pull/5110?src=pr&el=h1) Report
> Merging [#5110](https://codecov.io/gh/spacetelescope/jwst/pull/5110?src=pr&el=desc) into [master](https://codecov.io/gh/spacetelescope/jwst/commit/56ca9a3d210f058d5dce65b54435a73d43e8e7d1&el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `97.05%`.

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

```diff
@@            Coverage Diff             @@
##           master    #5110      +/-   ##
==========================================
+ Coverage   52.46%   52.52%   +0.06%     
==========================================
  Files         403      403              
  Lines       36181    36227      +46     
  Branches     5604     5616      +12     
==========================================
+ Hits        18984    19030      +46     
  Misses      16022    16022              
  Partials     1175     1175              
```

| Flag | Coverage Δ | |
|---|---|---|
| #unit | `52.52% <97.05%> (+0.06%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/spacetelescope/jwst/pull/5110?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [jwst/datamodels/util.py](https://codecov.io/gh/spacetelescope/jwst/pull/5110/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL3V0aWwucHk=) | `77.55% <87.50%> (+0.69%)` | :arrow_up: |
| [jwst/datamodels/dqflags.py](https://codecov.io/gh/spacetelescope/jwst/pull/5110/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL2RxZmxhZ3MucHk=) | `100.00% <100.00%> (ø)` | |
| [jwst/datamodels/fits\_support.py](https://codecov.io/gh/spacetelescope/jwst/pull/5110/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL2ZpdHNfc3VwcG9ydC5weQ==) | `83.42% <100.00%> (+1.76%)` | :arrow_up: |
| [jwst/datamodels/model\_base.py](https://codecov.io/gh/spacetelescope/jwst/pull/5110/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL21vZGVsX2Jhc2UucHk=) | `85.26% <100.00%> (-0.28%)` | :arrow_down: |
| [jwst/outlier\_detection/outlier\_detection.py](https://codecov.io/gh/spacetelescope/jwst/pull/5110/diff?src=pr&el=tree#diff-andzdC9vdXRsaWVyX2RldGVjdGlvbi9vdXRsaWVyX2RldGVjdGlvbi5weQ==) | `40.27% <100.00%> (ø)` | |
| [jwst/datamodels/properties.py](https://codecov.io/gh/spacetelescope/jwst/pull/5110/diff?src=pr&el=tree#diff-andzdC9kYXRhbW9kZWxzL3Byb3BlcnRpZXMucHk=) | `80.22% <0.00%> (+0.57%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/spacetelescope/jwst/pull/5110?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/5110?src=pr&el=footer). Last update [56ca9a3...cba5997](https://codecov.io/gh/spacetelescope/jwst/pull/5110?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 6676b5649..f0484167f 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -70,7 +70,9 @@ datamodels
 
 - Added "PERSISTENCE" DQ flag definition. [#5137]
 
-- Fix nonsensical premature closing of FITS file of a ``DataModel``. [#4930]
+- Fixed nonsensical premature closing of FITS file of a ``DataModel``. [#4930]
+
+- Added a hash set/check to DataModel I/O to check whether schema traversal is necessary. [#5110]
 
 extract_1d
 ----------
diff --git a/jwst/datamodels/dqflags.py b/jwst/datamodels/dqflags.py
index 1fc478e68..4c235cb1e 100644
--- a/jwst/datamodels/dqflags.py
+++ b/jwst/datamodels/dqflags.py
@@ -105,3 +105,45 @@ def interpret_bit_flags(bit_flags, flip_bits=None):
         bit_flags_dm = multiple_replace(bit_flags, dm_flags)
 
     return ap_interpret_bit_flags(bit_flags_dm, flip_bits=flip_bits)
+
+
+def dqflags_to_mnemonics(dqflags, mnemonic_map=pixel):
+    """Interpret value as bit flags and return the mnemonics
+
+    Parameters
+    ----------
+    dqflags : int-like
+        The value to interpret as DQ flags
+
+    mnemonic_map: {str: int[,...]}
+        Dictionary associating the mnemonic string to an integer value
+        representing the set bit for that mnemonic.
+
+    Returns
+    -------
+    mnemonics : {str[,...]}
+        Set of mnemonics represented by the set bit flags
+
+    Examples
+    --------
+    >>> dqflags_to_mnemonics(1)
+    {'DO_NOT_USE'}
+
+    >>> dqflags_to_mnemonics(7)             #doctest: +SKIP
+    {'JUMP_DET', 'DO_NOT_USE', 'SATURATED'}
+
+    >>> dqflags_to_mnemonics(7) == {'JUMP_DET', 'DO_NOT_USE', 'SATURATED'}
+    True
+
+    >>> dqflags_to_mnemonics(1, mnemonic_map=pixel)
+    {'DO_NOT_USE'}
+
+    >>> dqflags_to_mnemonics(1, mnemonic_map=group)
+    {'DO_NOT_USE'}
+    """
+    mnemonics = {
+        mnemonic
+        for mnemonic, value in mnemonic_map.items()
+        if (dqflags & value)
+    }
+    return mnemonics
diff --git a/jwst/datamodels/fits_support.py b/jwst/datamodels/fits_support.py
index e0e448ae8..ac18348d3 100644
--- a/jwst/datamodels/fits_support.py
+++ b/jwst/datamodels/fits_support.py
@@ -1,11 +1,14 @@
 import datetime
+import hashlib
 import os
-import re
 from pkg_resources import parse_version
+import re
+import warnings
 
 import numpy as np
 from astropy.io import fits
 from astropy import time
+from astropy.utils.exceptions import AstropyWarning
 import asdf
 from asdf import fits_embed
 from asdf import resolver
@@ -64,6 +67,9 @@ _keyword_indices = [
     ('s', 27, ' ABCDEFGHIJKLMNOPQRSTUVWXYZ')
     ]
 
+# Key where the FITS hash is stored in the ASDF tree
+FITS_HASH_KEY = '_fits_hash'
+
 
 def _get_indexed_keyword(keyword, i):
     for (sub, max, r) in _keyword_indices:
@@ -362,10 +368,6 @@ def _save_extra_fits(hdulist, tree):
                     continue
                 hdu.header.append((key, val, comment), end=True)
 
-    # Remove extra_fits so it is not written to the asdf extension
-    if 'extra_fits' in tree:
-        del tree['extra_fits']
-
 
 def _save_history(hdulist, tree):
     if 'history' not in tree:
@@ -395,6 +397,9 @@ def to_fits(tree, schema):
     _save_extra_fits(hdulist, tree)
     _save_history(hdulist, tree)
 
+    # Store the FITS hash in the tree
+    tree[FITS_HASH_KEY] = fits_hash(hdulist)
+
     asdf = fits_embed.AsdfInFits(hdulist, tree)
     return asdf
 
@@ -447,17 +452,25 @@ def _schema_has_fits_hdu(schema):
     return has_fits_hdu[0]
 
 
-def _load_from_schema(hdulist, schema, tree, context):
+def _load_from_schema(hdulist, schema, tree, context, skip_fits_update=False):
     known_keywords = {}
     known_datas = set()
 
+    # Check if there are any table HDU's. If not, this whole process
+    # can be skipped.
+    if skip_fits_update:
+        if not any(isinstance(hdu, fits.BinTableHDU) for hdu in hdulist if hdu.name != 'ASDF'):
+            log.debug('Skipping FITS updating completely.')
+            return known_keywords, known_datas
+        log.debug('Skipping FITS keyword updating except for BinTableHDU and its associated header keywords.')
+
     # Determine maximum EXTVER that could be used in finding named HDU's.
     # This is needed to constrain the loop over HDU's when resolving arrays.
     max_extver = max(hdu.ver for hdu in hdulist) if len(hdulist) else 0
 
     def callback(schema, path, combiner, ctx, recurse):
         result = None
-        if 'fits_keyword' in schema:
+        if not skip_fits_update and 'fits_keyword' in schema:
             fits_keyword = schema['fits_keyword']
             result = _fits_keyword_loader(
                 hdulist, fits_keyword, schema,
@@ -544,16 +557,44 @@ def _load_history(hdulist, tree):
         history['entries'].append(HistoryEntry({'description': entry}))
 
 
-def from_fits(hdulist, schema, context, **kwargs):
+def from_fits(hdulist, schema, context, skip_fits_update=None, **kwargs):
+    """Read model information from a FITS HDU list
+
+    Parameters
+    ----------
+    hdulist : astropy.io.fits.HDUList
+        The FITS HDUList
+
+    schema : dict
+        The schema defining the ASDF > FITS_KEYWORD, FITS_HDU mapping.
+
+    context: DataModel
+        The `DataModel` to update
+
+    skip_fits_update : bool or None
+        When `False`, models opened from FITS files will proceed
+        and load the FITS header values into the model.
+        When `True` and the FITS file has an ASDF extension, the
+        loading/validation of the FITS header will be skipped, loading
+        the model only from the ASDF extension.
+        When `None`, the value is taken from the environmental SKIP_FITS_UPDATE.
+        Otherwise, the default is `False`
+    """
     try:
         ff = from_fits_asdf(hdulist, **kwargs)
     except Exception as exc:
         raise exc.__class__("ERROR loading embedded ASDF: " + str(exc)) from exc
 
+    # Determine whether skipping the FITS loading can be done.
+    skip_fits_update = _verify_skip_fits_update(
+        skip_fits_update, hdulist, ff, context
+    )
+
     known_keywords, known_datas = _load_from_schema(
-        hdulist, schema, ff.tree, context
+        hdulist, schema, ff.tree, context, skip_fits_update=skip_fits_update
     )
-    _load_extra_fits(hdulist, known_keywords, known_datas, ff.tree)
+    if not skip_fits_update:
+        _load_extra_fits(hdulist, known_keywords, known_datas, ff.tree)
 
     _load_history(hdulist, ff.tree)
 
@@ -594,3 +635,94 @@ def from_fits_hdu(hdu, schema):
         data._coldefs._listeners = listeners
 
     return data
+
+
+def _verify_skip_fits_update(skip_fits_update, hdulist, asdf_struct, context):
+    """Ensure all conditions for skipping FITS updating are true
+
+    Returns True if either 1) the FITS hash in the asdf structure matches the input
+    FITS structure. Or 2) skipping has been explicitly asked for in `skip_fits_update`.
+
+    Parameters
+    ----------
+    skip_fits_update : bool
+        Regardless of FIT hash check, attempt to skip if requested.
+
+    hdulist : astropy.io.fits.HDUList
+        The input FITS information
+
+    asdf_struct : asdf.ASDFFile
+        The associated ASDF structure
+
+    context : DataModel
+        The DataModel being built.
+
+    Returns
+    -------
+    skip_fits_update : bool
+        All conditions are satisfied for skipping FITS updating.
+    """
+    if skip_fits_update is None:
+        skip_fits_update = util.get_envar_as_boolean('SKIP_FITS_UPDATE', None)
+
+    # If skipping has been explicitly disallowed, indicate as such.
+    if skip_fits_update is False:
+        return False
+
+    # Skipping has either been requested or has been left to be determined automatically.
+    # Continue checking conditions necessary for skipping.
+
+    # Need an already existing ASDF. If not, cannot skip.
+    if not len(asdf_struct.tree):
+        log.debug('No ASDF information found. Cannot skip updating from FITS headers.')
+        return False
+
+    # Ensure model types match
+    hdulist_class = util._class_from_model_type(hdulist)
+    if hdulist_class is None:
+        log.debug('Cannot determine model of the FITS file.'
+                  ' Cannot skip updating from FITS headers.')
+        return False
+    if not isinstance(context, hdulist_class):
+        log.debug(f'Input model {hdulist_class} does not match the'
+                  f' requested model {type(context)}.'
+                  ' Cannot skip updating from FITS headers.')
+        return False
+
+    # Check for FITS hash and compare to current. If equal, automatically skip.
+    if asdf_struct.tree.get(FITS_HASH_KEY, None) is not None:
+        if asdf_struct.tree[FITS_HASH_KEY] == fits_hash(hdulist):
+            log.debug('FITS hash matches. Skipping FITS updating.')
+            return True
+
+    # If skip only if explicitly requested.
+    return False if skip_fits_update is None else True
+
+
+def fits_hash(hdulist):
+    """Calculate a hash based on all HDU headers
+
+    Uses basic SHA-256 hash to calculate.
+
+    Parameters
+    ----------
+    hdulist : astropy.fits.HDUList
+        The FITS structure.
+
+    Returns
+    -------
+    fits_hash : str
+        The hash of all HDU headers.
+    """
+    fits_hash = hashlib.sha256()
+
+    # Ignore FITS header warnings, such as "Card is too long".
+    # Such issues are inconsequential to hash calculation.
+    with warnings.catch_warnings():
+        warnings.simplefilter('ignore', AstropyWarning)
+        fits_hash.update(''.join(
+            str(hdu.header)
+            for hdu in hdulist
+            if hdu.name != 'ASDF').encode()
+        )
+    return fits_hash.hexdigest()
diff --git a/jwst/datamodels/model_base.py b/jwst/datamodels/model_base.py
index aed48d31a..620597973 100644
--- a/jwst/datamodels/model_base.py
+++ b/jwst/datamodels/model_base.py
@@ -25,6 +25,7 @@ from . import fits_support
 from . import properties
 from . import schema as mschema
 from . import validate
+from .util import get_envar_as_boolean
 from ..lib import s3_utils
 
 from .history import HistoryList
@@ -93,12 +94,12 @@ class DataModel(properties.ObjectNode, ndmodel.NDModel):
         """
 
         # Override value of validation parameters if not explicitly set.
-        if not pass_invalid_values:
-            pass_invalid_values = self._get_envar_as_boolean("PASS_INVALID_VALUES",
+        if pass_invalid_values is None:
+            pass_invalid_values = get_envar_as_boolean("PASS_INVALID_VALUES",
                                                             False)
         self._pass_invalid_values = pass_invalid_values
-        if not strict_validation:
-            strict_validation = self._get_envar_as_boolean("STRICT_VALIDATION",
+        if strict_validation is None:
+            strict_validation = get_envar_as_boolean("STRICT_VALIDATION",
                                                           False)
         self._strict_validation = strict_validation
         self._ignore_missing_extensions = ignore_missing_extensions
@@ -315,32 +316,6 @@ class DataModel(properties.ObjectNode, ndmodel.NDModel):
             if fd is not None:
                 fd.close()
 
-    def _get_envar_as_boolean(self, name, default=False):
-        """Interpret an environmental as a boolean flag
-
-        Truth is any numeric value that is not 0 or
-        any of the following case-insensitive strings:
-
-        ('true', 't', 'yes', 'y')
-
-        Parameters
-        ----------
-        name : str
-            The name of the environmental variable to retrieve
-
-        default : bool
-            If the value cannot be determined, use as the default.
-        """
-        truths = ('true', 't', 'yes', 'y')
-        if name in os.environ:
-            value = os.environ[name]
-            try:
-                value = bool(int(value))
-            except ValueError:
-                return value.lower() in truths
-            return value
-        return default
-
     @staticmethod
     def clone(target, source, deepcopy=False, memo=None):
         if deepcopy:
diff --git a/jwst/datamodels/util.py b/jwst/datamodels/util.py
index 629005400..2434dd184 100644
--- a/jwst/datamodels/util.py
+++ b/jwst/datamodels/util.py
@@ -4,6 +4,7 @@ Various utility functions and data types
 
 import sys
 import warnings
+import os
 from os.path import basename
 
 import numpy as np
@@ -444,3 +445,36 @@ def create_history_entry(description, software=None):
     if software is not None:
         entry['software'] = software
     return entry
+
+
+def get_envar_as_boolean(name, default=False):
+    """Interpret an environmental as a boolean flag
+
+    Truth is any numeric value that is not 0 or
+    any of the following case-insensitive strings:
+
+    ('true', 't', 'yes', 'y')
+
+    Parameters
+    ----------
+    name : str
+        The name of the environmental variable to retrieve
+
+    default : bool
+        If the environmental variable cannot be accessed, use as the default.
+    """
+    truths = ('true', 't', 'yes', 'y')
+    falses = ('false', 'f', 'no', 'n')
+    if name in os.environ:
+        value = os.environ[name]
+        try:
+            value = bool(int(value))
+        except ValueError:
+            value_lowcase = value.lower()
+            if value_lowcase not in truths + falses:
+                raise ValueError(f'Cannot convert value "{value}" to boolean unambiguously.')
+            return value_lowcase in truths
+        return value
+
+    log.debug('Environmental "{name}" cannot be found. Using default value of "{default}".')
+    return default
diff --git a/jwst/outlier_detection/outlier_detection.py b/jwst/outlier_detection/outlier_detection.py
index 8cfc14855..abc189f2e 100644
--- a/jwst/outlier_detection/outlier_detection.py
+++ b/jwst/outlier_detection/outlier_detection.py
@@ -497,7 +497,7 @@ def flag_cr(sci_image, blot_image, **pars):
     log.debug("Pixels in cr_mask:  {}".format(count_cr))
 
     # Update the DQ array in the input image in place
-    np.bitwise_or(sci_image.dq, np.invert(cr_mask) * CRBIT, sci_image.dq)
+    sci_image.dq = np.bitwise_or(sci_image.dq, np.invert(cr_mask) * CRBIT)
 
 
 def abs_deriv(array):


Test patch:
diff --git a/jwst/datamodels/tests/test_models.py b/jwst/datamodels/tests/test_models.py
index 462bd3fa5..1995d1a7f 100644
--- a/jwst/datamodels/tests/test_models.py
+++ b/jwst/datamodels/tests/test_models.py
@@ -1,4 +1,3 @@
-from copy import copy
 import os
 from os import path as op
 import shutil
@@ -17,6 +16,7 @@ from jwst.datamodels import (DataModel, ImageModel, MaskModel, QuadModel,
                              MultiSlitModel, ModelContainer, SlitModel,
                              SlitDataModel, IFUImageModel, ABVegaOffsetModel)
 from jwst import datamodels
+from jwst.datamodels.util import get_envar_as_boolean
 from jwst.lib.file_utils import pushdir
 
 
@@ -49,6 +49,89 @@ def teardown():
     shutil.rmtree(TMP_DIR)
 
 
+@pytest.fixture
+def jail_environ():
+    """Lock changes to the environment"""
+    original = os.environ.copy()
+    try:
+        yield
+    finally:
+        os.environ = original
+
+
+@pytest.fixture(scope='module')
+def make_models(tmpdir_factory):
+    """Create basic models
+
+    Returns
+    -------
+    path_just_fits, path_model : (str, str)
+        `path_just_fits` is a FITS file of `DataModel` without the ASDF extension.
+        `path_model` is a FITS file of `DataModel` with the ASDF extension.
+    """
+    path = tmpdir_factory.mktemp('skip_fits_update')
+    path_just_fits = str(path / 'just_fits.fits')
+    path_model = str(path / 'model.fits')
+    primary_hdu = fits.PrimaryHDU()
+    primary_hdu.header['exp_type'] = 'NRC_IMAGE'
+    hduls = fits.HDUList([primary_hdu])
+    hduls.writeto(path_just_fits)
+    model = DataModel(hduls)
+    model.save(path_model)
+    return {
+        'just_fits': path_just_fits,
+        'model': path_model
+    }
+
+
+@pytest.mark.parametrize(
+    'which_file, skip_fits_update, expected_exp_type',
+    [
+        ('just_fits', None,  'FGS_DARK'),
+        ('just_fits', False, 'FGS_DARK'),
+        ('just_fits', True,  'FGS_DARK'),
+        ('model',     None,  'FGS_DARK'),
+        ('model',     False, 'FGS_DARK'),
+        ('model',     True,  'NRC_IMAGE')
+    ]
+)
+@pytest.mark.parametrize(
+    'open_func',
+    [DataModel, datamodels.open]
+)
+@pytest.mark.parametrize(
+    'use_env',
+    [False, True]
+)
+def test_skip_fits_update(jail_environ,
+                          use_env,
+                          make_models,
+                          open_func,
+                          which_file,
+                          skip_fits_update,
+                          expected_exp_type):
+    """Test skip_fits_update setting"""
+    # Setup the FITS file, modifying a header value
+    path = make_models[which_file]
+    hduls = fits.open(path)
+    hduls[0].header['exp_type'] = 'FGS_DARK'
+
+    # Decide how to skip. If using the environmental,
+    # set that and pass None to the open function.
+    try:
+        del os.environ['SKIP_FITS_UPDATE']
+    except KeyError:
+        # No need to worry, environmental doesn't exist anyways
+        pass
+    if use_env:
+        if skip_fits_update is not None:
+            os.environ['SKIP_FITS_UPDATE'] = str(skip_fits_update)
+            skip_fits_update = None
+
+    model = open_func(hduls, skip_fits_update=skip_fits_update)
+    assert model.meta.exposure.type == expected_exp_type
+
+
 def test_set_shape():
     with ImageModel((50, 50)) as dm:
         assert dm.shape == (50, 50)
@@ -182,7 +265,7 @@ def test_stringify():
 
     image = os.path.join(ROOT_DIR, "nircam_mask.fits")
     with MaskModel(image) as im:
-        assert str(im) ==  '<MaskModel(2048, 2048) from nircam_mask.fits>'
+        assert str(im) == '<MaskModel(2048, 2048) from nircam_mask.fits>'
 
 
 def test_section():
@@ -258,7 +341,7 @@ def test_default_value_anyof_schema():
 
 
 def test_imagemodel():
-    dims = (10,10)
+    dims = (10, 10)
     with ImageModel(dims) as dm:
         assert dm.data.shape == dims
         assert dm.err.shape == dims
@@ -279,10 +362,10 @@ def test_multislit():
         slit.data = np.random.rand(5, 5)
         slit.dm = np.random.rand(5, 5)
         slit.err = np.random.rand(5, 5)
-        assert slit.wavelength.shape == (0,0)
-        assert slit.pathloss_point.shape == (0,0)
-        assert slit.pathloss_uniform.shape == (0,0)
-        assert slit.barshadow.shape == (0,0)
+        assert slit.wavelength.shape == (0, 0)
+        assert slit.pathloss_point.shape == (0, 0)
+        assert slit.pathloss_uniform.shape == (0, 0)
+        assert slit.barshadow.shape == (0, 0)
 
 
 def test_secondary_shapes():
@@ -405,9 +488,9 @@ def test_initialize_arrays_with_arglist():
 def test_open_asdf_model(init):
     # Open an empty asdf file, pass extra arguments
     with DataModel(init=init, ignore_version_mismatch=False,
-                                ignore_unrecognized_tag=True) as model:
-        assert model._asdf._ignore_version_mismatch == False
-        assert model._asdf._ignore_unrecognized_tag == True
+                   ignore_unrecognized_tag=True) as model:
+        assert not model._asdf._ignore_version_mismatch
+        assert model._asdf._ignore_unrecognized_tag
 
 
 def test_open_asdf_model_s3(s3_root_dir):
@@ -713,22 +796,6 @@ def test_abvega_offset_model():
     model.close()
 
 
-@pytest.fixture(scope='module')
-def empty_model():
-    """Create an empy model"""
-    return DataModel()
-
-
-@pytest.fixture(scope='module')
-def jail_environ():
-    """Lock cheanges to the environment"""
-    original = copy(os.environ)
-    try:
-        yield
-    finally:
-        os.environ = original
-
-
 @pytest.mark.parametrize(
     'case, default, expected', [
         (None, False, False),
@@ -771,7 +838,7 @@ def jail_environ():
         ('YES', True, True),
     ]
 )
-def test_get_envar_as_boolean(case, default, expected, jail_environ, empty_model):
+def test_get_envar_as_boolean(case, default, expected, jail_environ):
     """Test various options to a boolean environmental variable"""
     var = '__TEST_GET_ENVAR_AS_BOOLEAN'
     if case is None:
@@ -782,7 +849,7 @@ def test_get_envar_as_boolean(case, default, expected, jail_environ, empty_model
     else:
         os.environ[var] = case
 
-    value = empty_model._get_envar_as_boolean(var, default=default)
+    value = get_envar_as_boolean(var, default=default)
     assert value == expected