JP-3721: Simplify ModelContainer #8831
Conversation
…assign_mtwcs to library" This reverts commit 018e3ce.
… duplicated utilities
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8831 +/- ##
===========================================
- Coverage 61.86% 12.46% -49.40%
===========================================
Files 377 377
Lines 38911 38814 -97
===========================================
- Hits 24071 4840 -19231
- Misses 14840 33974 +19134 ☔ View full report in Codecov by Sentry. |
|
Initial round of regression tests still showing several failures to track down |
|
The most recent changes fixed most of those failures. There is still a lot to do, including docs updates and the like, but I think it's time to get some feedback on the overall idea behind the new ModelContainer, so I'm requesting a review from some folks now even though this is staying in draft mode. |
emolter
requested review from
braingram,
perrygreenfield,
nden,
mcara,
mairanteodoro and
schlafly
| @@ -13,12 +13,12 @@ | |||
| "members": [ | |||
| { | |||
| "exptype": "SCIENCE", | |||
| "expname": "test.fits", | |||
| "expname": "test.fits" | |||
There was a problem hiding this comment.
Is one of the linting tools requiring this now. In them olden days, this sort of thing was sometimes recommended for eliminating errors when someone added new items to a list or dictionary (yes, it would then lead to a syntax error). I'm curious if this is now discouraged.
There was a problem hiding this comment.
For my json linter, at least, yes. I don't think these are being removed in Python
| else: | ||
| val = None | ||
|
|
||
| setattr(m.meta, attr, val) | ||
| self._models.append(m) | ||
|
|
||
| except IOError: | ||
| self.close() | ||
| raise | ||
|
|
||
| # Pull the whole association table into meta.asn_table |
There was a problem hiding this comment.
This comment now appears to be incorrect after the change.
| @@ -2859,6 +2856,7 @@ def do_extract1d( | |||
| if hasattr(meta_source, "int_times"): | |||
| output_model.int_times = meta_source.int_times.copy() | |||
|
|
|||
| print(meta_source) | |||
There was a problem hiding this comment.
Presumably this will be removed in the end.
| self.output_file = output_file | ||
|
|
||
| print("self.output_file = ", self.output_file) |
| @@ -82,6 +86,7 @@ def process(self, input, selfcal_list=None, bkg_list=None): | |||
| i.e., true self-calibration. | |||
| """ | |||
| input_sci, selfcal_list, bkg_list = _parse_inputs(input, selfcal_list, bkg_list) | |||
| print(input_sci, selfcal_list, bkg_list) | |||
| asn_data = json.load(f) | ||
| assert asn_data["products"][0]["members"][0]["expname"].split("/") == [DATADIR, "test.fits"] | ||
|
|
||
| # cehck that this can be opened |
Thanks for the review Perry. I agree we should add some documentation. I'm still wondering what people think of the new-look container overall - is it complementary to ModelLibrary, does it make sense as a data structure, do any methods still need to be removed? |
Resolves JP-3721
Resolves JP-2038
Closes #8738
Closes #5950
With the addition of ModelLibrary, ModelContainer can be substantially simplified. ModelLibrary certainly renders the
return_openandsave_openoptions obsolete, as well as theget_sectionsmethod, since if memory usage is a concern then ModelLibrary should be used instead. Some additional discussion about how the ModelContainer used to perform way to many tasks can be found on this innerspace page.During discussions related to JP-3715, it has become increasingly clear that the strictness of ModelLibrary and the additional borrow/shelve code required to access models is not necessary/desired for many use-cases. For example, the
calwebb_spec3pipeline currently makes extensive use of ModelContainer but does not have the same memory issues ascalwebb_image3does. Learning to use ModelLibrary may also be a nuisance for users manipulating relatively small datasets.Therefore, ModelContainer should not be removed, but instead become a lightweight, easy-to-use class for loading a list of models and association metadata. This PR proposed a container satisfying the following constraints:
datamodels.open()for asn-type data but is no longer itself a datamodelwith datamodels.open(asn.json) as containerhas the expected behaviorThis PR should also fix a long-standing issue, previously closed as won't-fix, that filenames inside associations containing directories (e.g., "test/file.fits") don't work in the level3 pipelines (but do work in the level2 pipelines).
Linked PR in stdatamodels: spacetelescope/stdatamodels#330
Linked PR in stpipe: spacetelescope/stpipe#190
Tasks
Build 11.3(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)docs/pageokify_regteststo update the truth filesnews fragment change types...
changes/<PR#>.general.rst: infrastructure or miscellaneous changechanges/<PR#>.docs.rstchanges/<PR#>.stpipe.rstchanges/<PR#>.datamodels.rstchanges/<PR#>.scripts.rstchanges/<PR#>.fits_generator.rstchanges/<PR#>.set_telescope_pointing.rstchanges/<PR#>.pipeline.rststage 1
changes/<PR#>.group_scale.rstchanges/<PR#>.dq_init.rstchanges/<PR#>.emicorr.rstchanges/<PR#>.saturation.rstchanges/<PR#>.ipc.rstchanges/<PR#>.firstframe.rstchanges/<PR#>.lastframe.rstchanges/<PR#>.reset.rstchanges/<PR#>.superbias.rstchanges/<PR#>.refpix.rstchanges/<PR#>.linearity.rstchanges/<PR#>.rscd.rstchanges/<PR#>.persistence.rstchanges/<PR#>.dark_current.rstchanges/<PR#>.charge_migration.rstchanges/<PR#>.jump.rstchanges/<PR#>.clean_flicker_noise.rstchanges/<PR#>.ramp_fitting.rstchanges/<PR#>.gain_scale.rststage 2
changes/<PR#>.assign_wcs.rstchanges/<PR#>.badpix_selfcal.rstchanges/<PR#>.msaflagopen.rstchanges/<PR#>.nsclean.rstchanges/<PR#>.imprint.rstchanges/<PR#>.background.rstchanges/<PR#>.extract_2d.rstchanges/<PR#>.master_background.rstchanges/<PR#>.wavecorr.rstchanges/<PR#>.srctype.rstchanges/<PR#>.straylight.rstchanges/<PR#>.wfss_contam.rstchanges/<PR#>.flatfield.rstchanges/<PR#>.fringe.rstchanges/<PR#>.pathloss.rstchanges/<PR#>.barshadow.rstchanges/<PR#>.photom.rstchanges/<PR#>.pixel_replace.rstchanges/<PR#>.resample_spec.rstchanges/<PR#>.residual_fringe.rstchanges/<PR#>.cube_build.rstchanges/<PR#>.extract_1d.rstchanges/<PR#>.resample.rststage 3
changes/<PR#>.assign_mtwcs.rstchanges/<PR#>.mrs_imatch.rstchanges/<PR#>.tweakreg.rstchanges/<PR#>.skymatch.rstchanges/<PR#>.exp_to_source.rstchanges/<PR#>.outlier_detection.rstchanges/<PR#>.tso_photometry.rstchanges/<PR#>.stack_refs.rstchanges/<PR#>.align_refs.rstchanges/<PR#>.klip.rstchanges/<PR#>.spectral_leak.rstchanges/<PR#>.source_catalog.rstchanges/<PR#>.combine_1d.rstchanges/<PR#>.ami.rstother
changes/<PR#>.wfs_combine.rstchanges/<PR#>.white_light.rstchanges/<PR#>.cube_skymatch.rstchanges/<PR#>.engdb_tools.rstchanges/<PR#>.guider_cds.rst