search

talensgj / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
1 stars 0 forks source link

Implement new Tikhonov code, to be verified by Antoine. #1

Closed talensgj closed 2 years ago

talensgj commented 2 years ago

I've copied over Antoine's revisions from jwst-mtl. I think the next steps should be as follows, and performed by @AntoineDarveau:

  1. These changes should be checked.
  2. The changes should be tested using a modified version of Antoine's notebook.
  3. The procedural code block in soss_extract.py function model_image() line 180 if tikfac is None: should be updated.

Only once these steps are complete and tested can the new Tikhonov code be merged back into the main SOSS development branch via this PR.

talensgj commented 2 years ago

I had a look at the revisions and have some points to raise.

First some minor comments on the procedural code:

  1. In estim_flux_first_order the parameter wave_bounds goes unused.
  2. I think estim_flux_first_order should be made part of engine_utils.py
  3. There should be 2 lines of whitespace after the function.

I also tried running the stage 2 pipeline. Previously, stage 2 failed due to the magnitude of the data changing in the photom step.

When I tried running stage 2 with the new Tikhonov code I first encountered a problem in the centroids. When calling the solver, it appears that some of the centroid values were out-of-bounds (>255) causing the code to crash. I'm unsure how this can happen, but it should clearly be looked into as a separate issue.

When I circumvented the solver, the new Tikhonov works just fine with the order of magnitude change in the values caused by the photom step, but the results are still rather puzzling. It appears the photom step has a significant impact on the shape of the extracted spectrum, though I'm not clear why this happens. I will post a comparison to slack shortly.

AntoineDarveau commented 2 years ago

I had a look at the revisions and have some points to raise.

First some minor comments on the procedural code:

  1. In estim_flux_first_order the parameter wave_bounds goes unused.

Oupsy

  1. I think estim_flux_first_order should be made part of engine_utils.py

My idea was to keep soss_engine and engine_utils general, so not SOSS-specific. Since estim_flux_first_order is very specific to the SOSS, I think it should not be in engine_utils.py. Maybe in soss_utils.py?

  1. There should be 2 lines of whitespace after the function.

Done

talensgj commented 2 years ago

I think the issues revealed when running stage 2 should be addressed on as a separate issue. So this branch can be merged now that the other points have been addressed.

njcuk9999 commented 2 years ago

agreed. @AntoineDarveau I think you will be able to merge this (or I can if you can't) - once you've confirmed you've made a note to address this issue in another branch (or tagged someone else to do this)

talensgj commented 2 years ago

I already made those issues. It's why I enabled the issues tab in the first place.