jwst icon indicating copy to clipboard operation
jwst copied to clipboard

Add ModelList to ModelContainer parents

Open tapastro opened this issue 2 years ago • 5 comments

This PR adds a parent ABC to ModelContainer, so that behavior in stpipe can be altered if step/pipeline input is a ModelContainer rather than a JwstDataModel.

Need to wait for stdatamodels release containing ModelList addition (currently expected to be 0.4.4.)

Checklist

  • [x] added entry in CHANGES.rst within the relevant release section
  • [ ] updated or added relevant tests
  • [ ] updated relevant documentation
  • [x] added relevant milestone
  • [x] added relevant label(s)

tapastro avatar Jul 28 '22 18:07 tapastro

I wrote this thinking Sequence existed only as a 'marker' parent class, i.e. we weren't using any of its functionality in ModelContainer. Do you know if this is true or not?

I tried to verify by putting all of my PRs on this project from stpipe/stdatmodels/jwst in one environment, and while I don't see any errors, there could be an error in how I put all of the new code in the conda environment... 🤷

tapastro avatar Jul 29 '22 14:07 tapastro

Ah, if Sequence is only a marker also, then should it be removed completely, since that is what ModelList is supposed to do?

stscieisenhamer avatar Jul 29 '22 16:07 stscieisenhamer

Ah, if Sequence is only a marker also, then should it be removed completely, since that is what ModelList is supposed to do?

I just wrote out a message saying that's what my PRs intended to do, then checked to see I left Sequence in the list of parent classes... I'll have to run the local tests again to see if Sequence is necessary.

tapastro avatar Jul 29 '22 16:07 tapastro

This PR is still marked as draft. When ready, please rebase and un-draft.

stscieisenhamer avatar Aug 10 '22 11:08 stscieisenhamer

This is no longer marked as draft, but it does rely on a new stdatamodels build before merging can be performed.

tapastro avatar Aug 10 '22 14:08 tapastro

is this still relevant?

stscieisenhamer avatar Mar 21 '24 17:03 stscieisenhamer