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.

Sign up for GitHub

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

Jump to bottom

Remove all uses of Step.__call__ #1499

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove all uses of Step.__call__ #1499

wants to merge 4 commits into from
+33 −43

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Nov 6, 2024 •
edited
Loading

This PR removes all uses of Step.__call__ (which will be deprecated in stpipe: spacetelescope/stpipe#204).

To find all uses Step.__call__ was intentionally broken and regression tests were run with this PR: https://github.com/spacetelescope/RegressionTests/actions/runs/11713132907

Regression tests (without breaking Step.__call__) at: https://github.com/spacetelescope/RegressionTests/actions/runs/11728757856
showed 1 unrelated failure.

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)
    • 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

@codecov Codecov
Copy link

codecov bot commented Nov 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 76.21%. Comparing base (1fe4cbc) to head (11806fe).

Files with missing lines Patch % Lines
romancal/pipeline/exposure_pipeline.py 0.00% 11 Missing ⚠️
romancal/pipeline/mosaic_pipeline.py 0.00% 7 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1499   +/-   ##
=======================================
  Coverage   76.21%   76.21%           
=======================================
  Files         115      115           
  Lines        7639     7639           
=======================================
  Hits         5822     5822           
  Misses       1817     1817

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

@braingram braingram changed the title Stpipe dunder call Remove all uses of Step.__call__ Nov 7, 2024
@braingram braingram mentioned this pull request Nov 7, 2024
Draft
7 tasks
@braingram braingram marked this pull request as ready for review November 7, 2024 18:52
@braingram braingram requested a review from a team as a code owner November 7, 2024 18:52
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Nov 7, 2024
braingram
braingram commented Nov 7, 2024
View reviewed changes
docs/roman/outlier_detection/outlier_examples.rst
@@ -14,7 +14,7 @@ the outlier detection step.
# read the file list into a ModelLibrary object
mc = ModelLibrary(["img_1.asdf", "img_2.asdf"])
step = OutlierDetectionStep()
step.process(mc)
Copy link
Collaborator Author

@braingram braingram Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not a use of __call__ but process isn't a recommended way to run a step.

braingram
braingram commented Nov 7, 2024
View reviewed changes
docs/roman/tweakreg/tweakreg_examples.rst
@@ -34,7 +34,7 @@ or a Roman datamodel `ImageModel`.
step.save_abs_catalog = True # save the catalog data used for absolute astrometry
step.abs_refcat = 'GAIADR3' # use Gaia DR3 for absolute astrometry
step.catalog_path = '/path/for/the/abs/catalog' # save the Gaia catalog to this path
step.call([img])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This use of call didn't use the step instance so the custom parameters were ignored. I changed the other uses in this file to run to match.

@braingram
Copy link
Collaborator Author

braingram commented Nov 7, 2024 •
edited
Loading

Thanks @ddavis-stsci for mentioning the docs. I found a few more uses fixed in b697b4f

In the same commit I also made a few other docs fixes:

  • removed uses of Step.process
  • switched a step.call to step.run since the example expected instance attributes to be used

@ddavis-stsci
Copy link
Collaborator

Which version of stpipe are you using. I've got stpipe 0.7.0 and I am not getting very far,
from romancal.pipeline import ExposurePipeline
2024-11-07 15:44:14,254 - stpipe - WARNING - WARNING: LOCAL JWST PRD VERSION PRDOPSSOC-065 DOESN'T MATCH THE CURRENT ONLINE VERSION PRDOPSSOC-067
Please consider updating pysiaf, e.g. pip install --upgrade pysiaf or conda update pysiaf

result = ExposurePipeline.run('rsim/r0000101001001001001_0001_wfi01_uncal.asdf')
Traceback (most recent call last):
File "", line 1, in
File "/Users/ddavis/miniconda3/envs/rcal_dev/lib/python3.11/site-packages/stpipe/step.py", line 424, in run
with log.record_logs(formatter=self._log_records_formatter) as log_records:
^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute '_log_records_formatter'

@braingram
Copy link
Collaborator Author

Which version of stpipe are you using. I've got stpipe 0.7.0 and I am not getting very far, from romancal.pipeline import ExposurePipeline 2024-11-07 15:44:14,254 - stpipe - WARNING - WARNING: LOCAL JWST PRD VERSION PRDOPSSOC-065 DOESN'T MATCH THE CURRENT ONLINE VERSION PRDOPSSOC-067 Please consider updating pysiaf, e.g. pip install --upgrade pysiaf or conda update pysiaf

result = ExposurePipeline.run('rsim/r0000101001001001001_0001_wfi01_uncal.asdf')
Traceback (most recent call last):
File "", line 1, in
File "/Users/ddavis/miniconda3/envs/rcal_dev/lib/python3.11/site-packages/stpipe/step.py", line 424, in run
with log.record_logs(formatter=self._log_records_formatter) as log_records:
^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute '_log_records_formatter'

The CI is also using 0.7.0. I suspect there is something wrong with your environment. Is it a new environment?

@ddavis-stsci
Copy link
Collaborator

I did a complete reinstall and get the same error. If I use call it works as expected.

@braingram
Copy link
Collaborator Author

braingram commented Nov 7, 2024 •
edited
Loading

Oh I missed the command you're running.
ExposurePipeline.run('rsim/r0000101001001001001_0001_wfi01_uncal.asdf')
That isn't a valid command on main or with this PR. run is an instance method (not a class method) so you need to make an instance of ExposurePipeline. Something like:

pipeline = ExposurePipeline()
pipeline.run('rsim/r0000101001001001001_0001_wfi01_uncal.asdf')

Alternatively you can use call (with this PR or on main):

ExposurePipeline.call('rsim/r0000101001001001001_0001_wfi01_uncal.asdf')

See the docs for more details:

@schlafly
Copy link
Collaborator

Dave, if you're happy here, we could merge this for B16. They have been trying to standardize the ways a step gets called in stpipe and we're the remaining holdout, so it would be good to get this in if possible.

@ddavis-stsci
Copy link
Collaborator

ddavis-stsci commented Nov 12, 2024 via email

@braingram
Copy link
Collaborator Author

Sure go ahead - we can fix the docs after the release.

Thanks. Are there more docs that need updating (beyond what's updated in this PR)?

@schlafly
Copy link
Collaborator

Some questions about whether this is a B16 or B17 merge; if we merged this in B17, would we want to do an stpipe upper pin for the release so that stpipe development can proceed with deprecating and removing this?

@braingram
Copy link
Collaborator Author

Some questions about whether this is a B16 or B17 merge; if we merged this in B17, would we want to do an stpipe upper pin for the release so that stpipe development can proceed with deprecating and removing this?

That sounds reasonable to me. @zacharyburnett is it part of the "release" to add lower and upper pins for stpipe (like in

romancal/pyproject.toml

Line 33 in c1d143a

"stpipe >=0.7.0,<0.8.0",
)?

@zacharyburnett
Copy link
Collaborator

I would say so, that should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddavis-stsci ddavis-stsci Awaiting requested review from ddavis-stsci

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation pipeline stpipe testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@braingram @ddavis-stsci @schlafly @zacharyburnett