Rcal 954 Remove jump code from rcal tree #1534

ddavis-stsci · 2024-11-22T13:57:02Z

Resolves RCAL-954

Closes #1518

This PR removes the old jump code for even ramps and updates the documentation.

Tasks

request a review from someone specific, to avoid making the maintainers review every PR
add a build milestone, i.e. 24Q4_B15 (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)
- [ x] 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#>.associations.rst
changes/<PR#>.scripts.rst
changes/<PR#>.mosaic_pipeline.rst
changes/<PR#>.patch_match.rst

steps

changes/<PR#>.dq_init.rst
changes/<PR#>.saturation.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.jump_detection.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.assign_wcs.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.photom.rst
changes/<PR#>.flux.rst
changes/<PR#>.source_detection.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.resample.rst
changes/<PR#>.source_catalog.rst

The regression tests are at https://github.com/spacetelescope/RegressionTests/actions/runs/11972755984 the one failure is for the ALL SATURNATED case where the truth file will need to be updated for the removal of the cal_step 'jump"

docs/roman/pipeline_run.rst

docs/roman/stpipe/call_via_call.rst

codecov · 2024-11-22T14:40:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.00%. Comparing base (6cb4b51) to head (7f75c72).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1534      +/-   ##
==========================================
- Coverage   78.11%   78.00%   -0.12%     
==========================================
  Files         117      115       -2     
  Lines        7639     7555      -84     
==========================================
- Hits         5967     5893      -74     
+ Misses       1672     1662      -10

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

mairanteodoro

Overall, looks good, @ddavis-stsci! However, I agree with @braingram that we should stick with the recommended call instead of run.

changes/1534.general.rst

docs/roman/stpipe/index.rst

docs/roman/pipeline_run.rst

braingram

Thanks for working on this. I left a few more requested changes.

braingram · 2024-12-10T20:21:20Z

docs/roman/pipeline_run.rst

-You can execute a pipeline or a step from within python by using the
-``call`` method of the class.
-
-The ``call`` method creates a new instance of the class and runs the pipeline or
-step. Optional parameter settings can be specified by via keyword arguments or


Please restore this section.

braingram · 2024-12-10T20:22:17Z

docs/roman/pipeline_run.rst

+ elp = ExposurePipeline()
+ result = elp.call('r0000101001001001001_0001_wfi01_uncal.asdf')


Suggested change

elp = ExposurePipeline()

result = elp.call('r0000101001001001001_0001_wfi01_uncal.asdf')

elp = ExposurePipeline()

result = elp.call('r0000101001001001001_0001_wfi01_uncal.asdf')

call is a class method and doesn't require an instance. Please restore the code from main.

braingram · 2024-12-10T20:22:32Z

docs/roman/pipeline_run.rst

+ linearity =  LinearityStep()
+ result = linearity.call('r0000101001001001001_0001_wfi01_uncal.asdf')


Suggested change

linearity = LinearityStep()

result = linearity.call('r0000101001001001001_0001_wfi01_uncal.asdf')

linearity = LinearityStep()

result = linearity.call('r0000101001001001001_0001_wfi01_uncal.asdf')

Same here, please restore the code from main.

braingram · 2024-12-10T20:24:20Z

docs/roman/pipeline_run.rst

@@ -95,13 +94,11 @@ For the mosaic level pipeline and steps,
 ::

 from romancal.pipeline import MosaicPipeline
- result = ExposurePipeline.call('r0000101001001001001_asn.json')
+ mosp = MosaicPipeline('r0000101001001001001_asn.json')


Suggested change

mosp = MosaicPipeline('r0000101001001001001_asn.json')

result = MosaicPipeline.call('r0000101001001001001_asn.json')

Thanks for switching this to MosaicPipeline. I think from the text above that this should be a call as it's describing the pipeline results.

braingram · 2024-12-10T20:25:03Z

docs/roman/pipeline_run.rst


 from romancal.skymatch import SkyMatchStep
- result = SkyMatchStep.call('r0000101001001001001_asn.json')
-
+ skymatch = SkyMatchStep.call('r0000101001001001001_asn.json')


Suggested change

skymatch = SkyMatchStep.call('r0000101001001001001_asn.json')

result = SkyMatchStep.call('r0000101001001001001_asn.json')

Assigning to skymatch is misleading as call is not returning a SkyMatchStep instance.

braingram · 2024-12-10T20:29:26Z

docs/roman/stpipe/user_pipeline.rst


 From Python
 -----------

 Once the pipeline has been configured (as above) it can be executed
 using run.

-    pipe.run(input_data)
+    pipe(input_data)


Suggested change

pipe(input_data)

pipe.run(input_data)

Using pipe(input_data) executes pipe.__call__ (which is deprecated). Please restore the example from main pipe.run.

braingram · 2024-12-10T20:29:39Z

docs/roman/stpipe/user_step.rst



-call()


Please restore this section.

braingram · 2024-12-10T20:29:52Z

docs/roman/stpipe/user_step.rst

@@ -233,11 +198,11 @@ example is::
 `input` in this case can be a asdf file containing the appropriate data, or the output
 of a previously run step/pipeline, which is an instance of a particular :ref:`datamodel<datamodels>`.

-Unlike the ``call`` class method, there is no parameter initialization that


Please restore this section.

braingram · 2024-12-10T20:31:51Z

docs/roman/references_general/references_general.rst

@@ -76,20 +72,12 @@ documentation on each reference file.
 +--------------------------------------------------+---------------------------------------------+
 | :ref:`FLAT <flat_reffile>`                       | :ref:`flatfield <flatfield_step>`           |
 +--------------------------------------------------+---------------------------------------------+
-| :ref:`GAIN <gain_reffile>`                       | :ref:`jump_detection <jump_step>`           |


Please restore this. The GAIN reference file is still used by the ramp step.

This is still not correct. The table is not formatted and GAIN and READNOISE are missing from the lower tables:
https://roman-pipeline--1534.org.readthedocs.build/en/1534/roman/references_general/references_general.html#reference-file-types

@ddavis-stsci would you address this issue?

braingram · 2024-12-10T20:32:02Z

docs/roman/references_general/references_general.rst

 | :ref:`LINEARITY <linearity_reffile>`             | :ref:`linearity <linearity_step>`           |
 +--------------------------------------------------+---------------------------------------------+
 | :ref:`MASK <mask_reffile>`                       | :ref:`dq_init <dq_init_step>`               |
 +--------------------------------------------------+---------------------------------------------+
 | :ref:`PHOTOM <photom_reffile>`                   | :ref:`photom <photom_step>`                 |
 +--------------------------------------------------+---------------------------------------------+
-| :ref:`READNOISE <readnoise_reffile>`             | :ref:`jump_detection <jump_step>`           |


Same as above with GAIN.

for more information, see https://pre-commit.ci

ddavis-stsci · 2024-12-11T16:46:37Z

Since there seem to be no more comment I'm merging this. Any issues can be handled in a future ticket.

braingram · 2024-12-11T16:47:33Z

Was this PR approved? I didn't approve it.

ddavis-stsci added documentation Improvements or additions to documentation jump labels Nov 22, 2024

ddavis-stsci added this to the 25Q2_B17 milestone Nov 22, 2024

ddavis-stsci requested a review from mairanteodoro November 22, 2024 13:57

ddavis-stsci self-assigned this Nov 22, 2024

ddavis-stsci requested a review from a team as a code owner November 22, 2024 13:57

github-actions bot added testing pipeline stpipe regression_testing associations Wide Field Instrument (WFI) labels Nov 22, 2024

ddavis-stsci removed pipeline stpipe associations labels Nov 22, 2024

github-actions bot added pipeline stpipe associations labels Nov 22, 2024

ddavis-stsci force-pushed the rcal-954_dsd branch 2 times, most recently from ec4752a to 9d5fbe2 Compare November 22, 2024 14:11

braingram reviewed Nov 22, 2024

View reviewed changes

docs/roman/pipeline_run.rst Outdated Show resolved Hide resolved

braingram reviewed Nov 22, 2024

View reviewed changes

docs/roman/pipeline_run.rst Outdated Show resolved Hide resolved

braingram reviewed Nov 22, 2024

View reviewed changes

docs/roman/stpipe/call_via_call.rst Outdated Show resolved Hide resolved

ddavis-stsci force-pushed the rcal-954_dsd branch 2 times, most recently from 69201cd to c8d37cd Compare December 2, 2024 18:59

mairanteodoro reviewed Dec 3, 2024

View reviewed changes

changes/1534.general.rst Outdated Show resolved Hide resolved

docs/roman/stpipe/index.rst Outdated Show resolved Hide resolved

ddavis-stsci enabled auto-merge (squash) December 4, 2024 13:15

braingram requested changes Dec 4, 2024

View reviewed changes

docs/roman/pipeline_run.rst Outdated Show resolved Hide resolved

github-actions bot added the dependencies Pull requests that update a dependency file label Dec 9, 2024

ddavis-stsci force-pushed the rcal-954_dsd branch from 24a6cc8 to 2e17d56 Compare December 10, 2024 20:16

braingram requested changes Dec 10, 2024

View reviewed changes

ddavis-stsci force-pushed the rcal-954_dsd branch 2 times, most recently from 4a38d2d to 11a48cd Compare December 10, 2024 21:29

ddavis-stsci and others added 18 commits December 11, 2024 07:37

rcal-954 Initial removal of jump code

b50a7db

rcal-954 Update git ignore for emacs backup files

531a27a

rcal-954 Add change text

9c143f2

rcal-954 Change congif file name in user pipeline docs

eadd35c

rcal-954 updates for run

3b9db97

rcal-954 remove unused ref in docs

a02a18d

rcal-954 Resolve merge conflicts

3564dea

rcal-954 Update git ignore for emacs backup files

211dd88

rcal-954 Resolve merge conflicts

1a1fc66

rcal-954 updates for run

419d74c

[pre-commit.ci] auto fixes from pre-commit.com hooks

080b5de

for more information, see https://pre-commit.ci

Update 1534.general.rst

9783571

rcal-954 restore

ec87f6e

rcal-954 More review updates

70b28f7

rcal-954 user requests

490ec05

rcal-954 user requests

b4ec081

rcal-954 user requests

8843126

rcal-954 user requests

733ae50

ddavis-stsci force-pushed the rcal-954_dsd branch from 036eb4f to 733ae50 Compare December 11, 2024 12:37

[pre-commit.ci] auto fixes from pre-commit.com hooks

7f75c72

for more information, see https://pre-commit.ci

ddavis-stsci disabled auto-merge December 11, 2024 12:43

ddavis-stsci merged commit e2d5fb9 into spacetelescope:main Dec 11, 2024
29 of 30 checks passed

ddavis-stsci deleted the rcal-954_dsd branch December 11, 2024 16:46

braingram mentioned this pull request Dec 11, 2024

Reference file table formatting is broken and GAIN and READNOISE files are missing #1559

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rcal 954 Remove jump code from rcal tree #1534

Rcal 954 Remove jump code from rcal tree #1534

ddavis-stsci commented Nov 22, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

mairanteodoro left a comment

braingram left a comment

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 10, 2024

braingram Dec 11, 2024

braingram Dec 11, 2024

braingram Dec 10, 2024

ddavis-stsci commented Dec 11, 2024

braingram commented Dec 11, 2024

		elp = ExposurePipeline()
		result = elp.call('r0000101001001001001_0001_wfi01_uncal.asdf')

		linearity = LinearityStep()
		result = linearity.call('r0000101001001001001_0001_wfi01_uncal.asdf')

	mosp = MosaicPipeline('r0000101001001001001_asn.json')
	result = MosaicPipeline.call('r0000101001001001001_asn.json')

	skymatch = SkyMatchStep.call('r0000101001001001001_asn.json')
	result = SkyMatchStep.call('r0000101001001001001_asn.json')



		call()

Rcal 954 Remove jump code from rcal tree #1534

Rcal 954 Remove jump code from rcal tree #1534

Conversation

ddavis-stsci commented Nov 22, 2024

Tasks

steps

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

mairanteodoro left a comment

Choose a reason for hiding this comment

braingram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddavis-stsci commented Dec 11, 2024

braingram commented Dec 11, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading