spacetelescope / stpipe

https://stpipe.readthedocs.io
Other
3 stars 25 forks source link

`call` and `run` changes #165

Open braingram opened 3 months ago

braingram commented 3 months ago

xlinking https://github.com/spacetelescope/jwst/issues/8130

Some discussion is underway to clean up the Step methods including call run and __call__ to clarify which methods create new instances, fetch reference files, overwrite parameters etc.

One currently discussed plan would be to:

I'd make an argument that part of the confusion is that call is a classmethod yet a user can call it on an instance. Replacing the classmethod with a separate function would help clean this up maybe create_and_run(step_class, ...) where the method can check that step_class is not an instance.

run could be renamed to maybe run_without_crds_parameters to make it explicit that the crds parameter files will be ignored.

Pinging @jdavies-st for comments.

jdavies-st commented 3 months ago

I think deprecating __call__ is probably not a good idea, as that's generally what one does with class instances - calls them. I think what __call__ points to, i.e. self.run is problematic, but only because the class constructor doesn't take (generally) input data as a required input.

Agree completely that Step.call is a class factory and should be a function, not a class method. It is confusing to users to allow it on an instance. I would not be for adding more non-pythonic methods of creating steps and running them. Keep it as one would expect it to be in Python.

Those are just some inital thoughts. Not new by any means, but things we knew were bad 6 years ago.

braingram commented 3 months ago

xref: https://github.com/spacetelescope/stpipe/issues/9