spacetelescope / stdatamodels

https://stdatamodels.readthedocs.io
Other
5 stars 25 forks source link

JP-3721: Simplify ModelContainer #330

Closed emolter closed 1 month ago

emolter commented 2 months ago

Helps to resolve JP-3721

Helps to close spacetelescope/jwst:8738

This PR enables the JWST ModelContainer to no longer inherit from DataModel as part of an effort to make ModelContainer's usage narrower and easier to understand.

Tasks

news fragment change types... - ``changes/.feature.rst``: new feature - ``changes/.bugfix.rst``: fixes an issue - ``changes/.doc.rst``: documentation change - ``changes/.removal.rst``: deprecation or removal of public API - ``changes/.misc.rst``: infrastructure or miscellaneous change
codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.55%. Comparing base (fd6be8d) to head (9bcaae1). Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #330 +/- ## ======================================= Coverage 66.55% 66.55% ======================================= Files 102 102 Lines 5456 5457 +1 ======================================= + Hits 3631 3632 +1 Misses 1825 1825 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

emolter commented 1 month ago

started jwst regtests here

braingram commented 1 month ago

This PR enables the JWST ModelContainer to no longer inherit from DataModel and to no longer require its own save method, as part of an effort to make ModelContainer's usage narrower and easier to understand.

I follow the changes motivated by the inheritance change. Since ModelContainer is no longer a JwstDataModel subclass it won't enter: https://github.com/spacetelescope/stdatamodels/blob/852c8d71a57d3c0f9f2eb1d12a7b89d77461a38a/src/stdatamodels/jwst/datamodels/util.py#L103-L105 by switching list to Sequence here: https://github.com/spacetelescope/stdatamodels/blob/852c8d71a57d3c0f9f2eb1d12a7b89d77461a38a/src/stdatamodels/jwst/datamodels/util.py#L156-L162 dm.open(container) will do the same thing as before (call ModelContainer(container)).

I don't following the no longer require its own save bit. Is that just in reference to AbstractDataModel.save?

emolter commented 1 month ago

I don't following the no longer require its own save bit. Is that just in reference to AbstractDataModel.save?

Yes, and it hasn't got anything to do with stdatamodels specifically, as you correctly point out. I will remove it from the summary

emolter commented 1 month ago

regression tests passed. @braingram I don't have permissions to request additional reviewers nor merge in this repository, feel free to do either at your convenience