8000 JP-1679: Skip serializing None in datamodels by jdavies-st · Pull Request #5371 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-1679: Skip serializing None in datamodels #5371

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
Merged
merged 9 commits into from
Oct 2, 2020
Merged

JP-1679: Skip serializing None in datamodels #5371

merged 9 commits into from
Oct 2, 2020

Conversation

jdavies-st
Copy link
Collaborator
@jdavies-st jdavies-st commented Sep 29, 2020 •
edited
Loading

From asdf 2.8 onwards, None will not be stripped from the ASDF tree before serialization. But jwst.datamodels currently depends on this behavior for a variety of reasons.

Lots of pipeline steps populate things in meta with None. If we don't strip None, it causes a validation error for many of the items in our schemas, because None is not an allowed type. The schema is expecting a string or number, not None.

So the solution is to strip any None values before save, and avoid populating attributes with None internally.

Other clean up:

  • warnings were being emitted by ModelContainer for instruments that were not NIRCam. A useless warning that I put in several years ago to detect malformed NIRCam simulated data. No longer needed.

HT: @eslavich for way forward and the first commit. 🚀

Resolves #5314 / JP-1679

10000

@jdavies-st jdavies-st added this to the Build 7.7 milestone Sep 29, 2020
@jdavies-st jdavies-st self-assigned this Sep 29, 2020
@jdavies-st jdavies-st force-pushed the eslavich-skip-serializing-none branch from 683617f to afc109b Compare September 29, 2020 21:25
eslavich
eslavich approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor
@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.

One extremely minor optional comment. LGTM!

jwst/datamodels/container.py
@@ -348,12 +347,6 @@ def _assign_group_ids(self):
''.join(params[3:6]), params[6]]))
model.meta.group_id = group_id
except TypeError:
params_dict = dict(zip(unique_exposure_parameters, params))
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the issue here? Not that I'd miss this code, just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is a very unnecessary warning that applies to all data that isn't NIRCam. So you are cursed to get this warning if you're not using NIRCam data. Not a good warning.

@codecov
Copy link
codecov bot commented Sep 29, 2020 •
edited
Loading

Codecov Report

Merging #5371 into master will decrease coverage by 0.02%.
The diff coverage is 45.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5371      +/-   ##
==========================================
- Coverage   52.87%   52.85%   -0.03%     
==========================================
  Files         409      409              
  Lines       37093    37116      +23     
  Branches     5759     5765       +6     
==========================================
+ Hits        19613    19617       +4     
- Misses      16231    16248      +17     
- Partials     1249     1251       +2
Flag Coverage Δ
#unit 52.85% <45.16%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jwst/datamodels/container.py 74.31% <ø> (-0.55%) ⬇️
jwst/resample/resample_spec_step.py 75.00% <0.00%> (-2.22%) ⬇️
...ter_background/master_background_nrs_slits_step.py 18.18% <6.25%> (-2.31%) ⬇️
jwst/datamodels/model_base.py 85.45% <100.00%> (+0.27%) ⬆️
jwst/flatfield/flat_field.py 19.78% <100.00%> (ø)
jwst/resample/resample_spec.py 82.75% <100.00%> (+0.08%) ⬆️
jwst/datamodels/reference.py 83.33% <0.00%> (-4.17%) ⬇️

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 4188b6a...b3a9044. Read the comment docs.

@eslavich eslavich mentioned this pull request Sep 29, 2020
Merged
@hbushouse
Copy link
Collaborator

Sorry, I updated the change log this morning for my own changes in another PR, so now you've got conflicts.

@jdavies-st jdavies-st force-pushed the eslavich-skip-serializing-none branch from f3a71b4 to 29f142e Compare September 30, 2020 14:40
@jdavies-st
Copy link
Collaborator Author

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.

@jdavies-st
Copy link
Collaborator Author
jdavies-st commented Oct 1, 2020 •
edited
Loading

This PR cannot be merged until astropy 4.0.2 is released which contains this PR:

astropy/astropy#10674

Without the above change, 168 of our regression tests fail. The transforms used in the WCS cannot be serialized properly.

@jdavies-st
Copy link
Collaborator Author

So, after discussion with @eslavich and @nden today, it looks like this PR is fine as is and can be merged. We will only see failures if asdf 2.8 is released before the fix astropy/astropy#10674 makes it into an astropy release. Which should not happen, as that will cause all sorts of other problems for packages outside of jwst. So this is good as is, as we will continue to use it with asdf 2.7.1 but will be ready when asdf 2.8 is released.

@jdavies-st
Copy link
Collaborator Author
jdavies-st commented Oct 2, 2020 •
edited
Loading

It looks like the small changes in resampling I was seeing in some spectroscopic regression tests were not due floating point accuracy issues, but to the change in Tabular1D of not passing fill_value=None. I mistakenly thought that was the default value, but the default value is actually NaN. So I've reverted it back to fill_value=None, which causes the tabular model to extrapolate beyond the bounding box if needed. Running the regression tests again now to make sure everything is good.

@jdavies-st jdavies-st merged commit 0382b8d into spacetelescope:master Oct 2, 2020
@jdavies-st jdavies-st deleted the eslavich-skip-serializing-none branch October 2, 2020 19:41
@jdavies-st jdavies-st mentioned this pull request Oct 26, 2020
Closed
@jdavies-st jdavies-st mentioned this pull request Jan 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eslavich eslavich eslavich approved these changes

@hbushouse hbushouse hbushouse approved these changes

Assignees

@jdavies-st jdavies-st

Labels
ASDF datamodels
Projects
None yet
Milestone
Build 7.7
Development

Successfully merging this pull request may close these issues.

datamodels should not serialize null values
3 participants
@jdavies-st @hbushouse @eslavich
0