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-3695: Clean up unnecessary copies #8676

Merged
merged 104 commits into from
Sep 20, 2024

Conversation

penaguerrero
Copy link
Contributor

@penaguerrero penaguerrero commented Jul 25, 2024

Partially Resolves JP-3695

Closes #

This PR addresses removing the unnecessary copies of all steps in the Detector1 pipeline. The same needs to be done for the stage2 pipelines.

Copies are made at the _step.py level, only if the step is not skipped.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

Copy link
Contributor

@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

Some changes requested, and needs a changelog entry. We should be able to get this into the build!

The regression test will hopefully be replaced by benchmarks come the end of next build, but we can keep it in until they are functional.

@@ -79,7 +79,6 @@ def do_correction(input_model, emicorr_model, save_onthefly_reffile, **pars):
will operate. Valid parameters include:
save_intermediate_results - saves the output into a file and the
reference file (if created on-the-fly)
user_supplied_reffile - reference file supplied by the user
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed from the docs as well, if it's not referenced in the step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this line already, not sure why it still showed up. I double checked now and it is not there.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still listed in the documentation pages - that's where it also needs to be pulled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah! I see it now, thank you

@@ -39,8 +41,7 @@ def process(self, input):

# Try to open the input as a regular RampModel
try:
input_model = datamodels.RampModel(input)

input_model = datamodels.RampModel(step_input)
Copy link
Contributor

@tapastro tapastro Sep 20, 2024

Choose a reason for hiding this comment

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

The following lines (49 and 56) need to have input updated to step_input, it's causing the fgs regression test failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thank you!


# Before the ramp_fit() call, copy the input model ("_W" for weighting)
# for later reconstruction of the fitting array tuples.
input_model_W = copy.copy(input_model)
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this copy changes downstream behavior when the ramp_fit algorithm is 'OLS'. This is no longer the default algorithm in our tests, but a diff can be seen if setting the algorithm to 'OLS'. I think this part needs to be reverted, at least until we remove the 'OLS' algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted these lines.

@tapastro tapastro added this to the Build 11.1 milestone Sep 20, 2024
@penaguerrero
Copy link
Contributor Author

Some changes requested, and needs a changelog entry.
We had talked about this in the meetings and had decided that no log entry was needed since it did not change the behavior for users. Thinking about it a bit more, I will go ahead and add a log entry, for tracking purposes.

@tapastro
Copy link
Contributor

tapastro commented Sep 20, 2024

Some changes requested, and needs a changelog entry.

We had talked about this in the meetings and had decided that no log entry was needed since it did not change the behavior for users. Thinking about it a bit more, I will go ahead and add a log entry, for tracking purposes.

That's fine, you can mark it with the appropriate no-changelog tag then.

@tapastro
Copy link
Contributor

Latest regtest run was clean: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1741/

Merging!

@tapastro tapastro merged commit 4d60e7a into spacetelescope:master Sep 20, 2024
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment