search

oirlab / liger_iris_pipeline

Data Reduction System (DRS) for the Thirty Meter Telescope IRIS imager/spectrograph
https://oirlab.github.io/iris-pipeline
0 stars 3 forks source link

Rework datamodels to run with latest stpipe and stdatamodels #58

Closed zonca closed 1 week ago

zonca commented 2 weeks ago

@astrobc1 ok, I got at least 1 test working, so I know all the pieces work together fine.

Now I suggest we proceed like this:

1) you run the test locally pytest liger_iris_pipeline/tests/test_dark.py and make sure it works (report back here when done) 1) you review code changes in this pull request and provide some feedback, high level, at the moment I would like to make sure you understand what I implemented, we will improve the code later on. Please do not make code modifications yet, I would like to finalize this PR, merge it into main, then we can spin-off from there multiple pull requests.

in the meantime I am trying to fix Github Actions.

astrobc1 commented 2 weeks ago

@zonca test_dark.py passes now. I'm working on the second item as well as understanding the logic for instantiating a DataModel. I will report back here early this week.

astrobc1 commented 1 week ago

@zonca Overall the changes make sense and appear correct.

zonca commented 1 week ago

@astrobc1 thanks for the feedback, best way to give feeback is to do a review and reference the line of code instead of the commit itself, doing it from the https://github.com/oirlab/liger_iris_pipeline/pull/58/files files view. It is convenient because we can have separate threads and mark them completed when we are done analyzing an issue.

@zonca Overall the changes make sense and appear correct.

  • 2767799 If the FITS keyword "DATAMODL" is stored in all headers, can't we always rely on datamodels.open instead of the class explicitly so long as they are registered?

Until I have a good understanding of the code and know better, I always prefer to follow what JWST is doing. For some reason I don't know yet, JWST is now using the actual classes, see https://github.com/spacetelescope/jwst/blob/10501f22def0460b15df65cd5261601c6a71d7cf/jwst/dark_current/dark_current_step.py#L29 so I do the same.

  • fcc9cd0 How did you solve the output_ext not defined error?

Yes, I think it comes from here:

https://github.com/spacetelescope/jwst/blob/10501f22def0460b15df65cd5261601c6a71d7cf/jwst/stpipe/core.py#L20-L24

  • 44e285d Will we eventually swap out jwst.stpipe.core.JwstStep for stpipe.step.Step?

Yes, if we can we want to get rid of all dependencies on jwst, but it is not too bad, so for now let's go forward like this

  • 701a431 I see how this stems from an assumption that get_reference_file expects a filename, not a DataModel. Should we override get_reference_file to accept both?

There is something I do not understand here, maybe you can track it down, see here: https://github.com/spacetelescope/jwst/blob/10501f22def0460b15df65cd5261601c6a71d7cf/jwst/dark_current/dark_current_step.py#L32

they are doing the same, feeding the input model to get_reference_file, why does it work for them and not for us? I think that at some point in the class hierarchy we are using different classes but have not understood where.

zonca commented 1 week ago

@astrobc1 I created a dedicated issue about the get_reference_file problem and I am merging this PR. We will track it down later.