Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JP-3755: Remove unused options and add unit tests to SOSS extraction algorithm #9000

Merged
merged 79 commits into from
Jan 22, 2025

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Dec 10, 2024

Resolves JP-3755

Closes #8804

This PR addresses reducing the technical debt of the SOSS ATOCA algorithm. There are a lot of changed lines, so I'm annotating this PR with comments during a self-review. The high-level changes are:

  • Removed many unused functions, i.e., ones that were never called during a pipeline run
  • Removed many optional (and occasionally required) parameters from functions when they were either never changed from their default value, or always set to the same value (AND were not set-able by the user or by parameter reference files)
  • Added a unit testing suite, including a detector model (wave grid, trace profiles, kernels, throughput functions, mock data, masks, etc) scaled down by a factor of 10 in each spatial dimension. Added unit tests for most helper functions.
  • Fixed various bugs, usually found when unit-testing function edge cases. I'm attempting to annotate all of these, so it's clear where actual changes to output have occurred. (These obviously require the most scrutiny).
  • Many small refactors for clarity

There are several outstanding items to complete before this can be merged:

  • Validate that changes in behavior (e.g., differences in retrieved Tikhonov regularization factor) are ok, i.e., not adversely affecting science results.
  • Get several questions about intended / current behavior answered by instrument team members. edit: got the OK from Joe that the rest of the outstanding questions can wait and be handled separately if needed
  • Check whether some removed functions should be retained in the pipeline version of ATOCA, e.g., because they are particularly helpful for INS work on SOSS data (even though not used during pipeline runs). edit: Joe is unaware of any usages like this- as far as he knows, everyone uses the pipeline-level interface only.
  • Add documentation of the algorithm to the pipeline readthedocs page. edit: Nadia suggests this should be done by INS folks, and that it's fine to merge this without the documentation updates.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.datamodels.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.fits_generator.rst
  • changes/<PR#>.set_telescope_pointing.rst
  • changes/<PR#>.pipeline.rst

stage 1

  • changes/<PR#>.group_scale.rst
  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.emicorr.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.ipc.rst
  • changes/<PR#>.firstframe.rst
  • changes/<PR#>.lastframe.rst
  • changes/<PR#>.reset.rst
  • changes/<PR#>.superbias.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.rscd.rst
  • changes/<PR#>.persistence.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.charge_migration.rst
  • changes/<PR#>.jump.rst
  • changes/<PR#>.clean_flicker_noise.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.gain_scale.rst

stage 2

  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.badpix_selfcal.rst
  • changes/<PR#>.msaflagopen.rst
  • changes/<PR#>.nsclean.rst
  • changes/<PR#>.imprint.rst
  • changes/<PR#>.background.rst
  • changes/<PR#>.extract_2d.rst
  • changes/<PR#>.master_background.rst
  • changes/<PR#>.wavecorr.rst
  • changes/<PR#>.srctype.rst
  • changes/<PR#>.straylight.rst
  • changes/<PR#>.wfss_contam.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.fringe.rst
  • changes/<PR#>.pathloss.rst
  • changes/<PR#>.barshadow.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.pixel_replace.rst
  • changes/<PR#>.resample_spec.rst
  • changes/<PR#>.residual_fringe.rst
  • changes/<PR#>.cube_build.rst
  • changes/<PR#>.extract_1d.rst
  • changes/<PR#>.resample.rst

stage 3

  • changes/<PR#>.assign_mtwcs.rst
  • changes/<PR#>.mrs_imatch.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.exp_to_source.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.tso_photometry.rst
  • changes/<PR#>.stack_refs.rst
  • changes/<PR#>.align_refs.rst
  • changes/<PR#>.klip.rst
  • changes/<PR#>.spectral_leak.rst
  • changes/<PR#>.source_catalog.rst
  • changes/<PR#>.combine_1d.rst
  • changes/<PR#>.ami.rst

other

  • changes/<PR#>.wfs_combine.rst
  • changes/<PR#>.white_light.rst
  • changes/<PR#>.cube_skymatch.rst
  • changes/<PR#>.engdb_tools.rst
  • changes/<PR#>.guider_cds.rst

Copy link
Contributor

@hover2pi hover2pi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emolter Everything looks good to me! I tried to respond to all your comments that asked questions. I do need to do a little reading on how the ATOCA algorithm works from Antoine's paper and then may have better answers. Ultimately, if the results are within our tolerances after removing a whole bunch of stuff, I think you were successful. We'll be revisiting several aspects of the code in the coming year, particularly by replacing the specprofile and speckernel reference files with pastasoss functionality. As it stands, I think it's in great shape. Thanks!

@hover2pi
Copy link
Contributor

hover2pi commented Jan 9, 2025

@emolter Regarding the differences in the extracted order 2 spectra from the SUBSTRIP96 data between the two branches, I'll definitely take a closer look. It would be great to understand why it changed but ultimately order 2 is not really used from that data. Between the heavy contamination from order 1 at long wavelengths and the order 2 throughput dropping to almost 0 around 1.2um, it's not too worrisome.

@emolter
Copy link
Collaborator Author

emolter commented Jan 10, 2025

@tapastro
Copy link
Contributor

Pending some small discussions and clean regression tests, this looks good to me! 👏

@emolter emolter requested a review from hover2pi January 13, 2025 15:40
Copy link
Contributor

@hover2pi hover2pi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from looking to a few things on my end, everything looks good to me.

@emolter
Copy link
Collaborator Author

emolter commented Jan 14, 2025

regression test rerun after attempted fix: https://github.com/spacetelescope/RegressionTests/actions/runs/12758637636

All regression test failures are expected; they are limited to tests specific to NIRISS SOSS stage 3 extractions, and are due to numerical differences introduced by this PR. The outputs from the step have been validated by @hover2pi and I and are found to be in-line with the expectations of the INS team as far as we can tell.

The only remaining conversations that have not been marked as "Resolved" are discussing changes that are outside the scope of this PR, and follow-up issues/tickets have been created for those.

Marking this ready for review. @tapastro Do you see anything else that should be changed before merging?

Note that I did re-run the step on the two test datasets plotted above after the fix in 0003648 and the plots look identical. The bug was introduced by 8cec4ba, which was made after running these test datsets the first time, so the results should be the same after fixing, and indeed they are.

@emolter emolter marked this pull request as ready for review January 14, 2025 22:28
@emolter emolter requested review from a team as code owners January 14, 2025 22:28
@tapastro tapastro merged commit b2869fc into spacetelescope:main Jan 22, 2025
31 checks passed
@emolter emolter deleted the JP-3755 branch January 22, 2025 17:06
@emolter
Copy link
Collaborator Author

emolter commented Jan 22, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce technical debt of ATOCA/SOSS extraction code
7 participants