adding memory tests for detector 1 and the jump step

[penaguerrero]

Is part of JP-4780

This PR addresses the memory usage of detector 1 and the jump step for NIRSpec data. (A MIRI detector 1 test is already in the repo).

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.85%. Comparing base (19f7809) to head (1131e9f).
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8888   +/-   ##
=======================================
  Coverage   61.85%   61.85%           
=======================================
  Files         377      377           
  Lines       38877    38877           
=======================================
  Hits        24046    24046           
  Misses      14831    14831           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[emolter]

This test brings up a big question, which is, "how should we track memory usage of entire pipeline steps over time?"

The original intention with the MemoryThreshold context manager was to use it in situations where the memory usage of a routine can be analytically determined based on the size of the input. For example, its use in stcal outlier detection testing ensures that the median calculation never has to hold the entire input data cube in memory.

This test is a bit different - it is basically trying to ensure that future changes to an entire step do not increase memory usage "too much". While memory optimizations have been done for the step, specific functions are not being tested here, and the peak memory usage is not being determined from the inputs.

Implementing tests like this one is not necessarily wrong, but there is another way of going about this, which is to use some external program (you could imagine something similar to Codecov) that tracks memory usage and runtime of our regression tests and displays the output to the developer on their pull request. Two benefits of the latter approach are (1) we wouldn't need to add context managers or decorators to every test and (2) we wouldn't need to adjust the memory limit every time a PR changes memory usage for a good reason. We discussed back-and-forth at the hack day about these various options but did not make a decision.

Even if we do go down the route suggested by this PR of marking specific tests with a memory limit, there was disagreement at the hack day about how best to track the limit. One suggestion was to basically do it the way you've done it here, and then adjust the limit in the test along with the PR. Another suggestion was to keep the memory threshold in a file in artifactory such that it could be ok-ified if a PR changed it for a good reason. In both cases, it was noted that it's necessary to set a lower limit on memory as well, because otherwise we would forget to make the threshold stricter when additional memory optimizations are made. This last point somewhat argues against the use of the MemoryThreshold class.

Sorry for the long message, but I wanted to give you all the information since we had the relevant discussions without you. I think we need to wait on some decision-making about the general direction of memory and runtime testing before we start implementing this type of test.

Are there individual functions inside detector1 where you can determine the peak usage based on the input array size? If so, tests for those could be written right now.

Tagging @melanieclarke @tapastro @braingram