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-3562: Extend snowball core #8303

Merged
merged 20 commits into from
Mar 25, 2024

Conversation

mwregan2
Copy link
Contributor

@mwregan2 mwregan2 commented Feb 22, 2024

This PR addresses the need to add two new parameters for the Jump step that control the flagging of cores of snowballs in the next integration.
The two parameters exist in the STCAL code and just need an external interface

Checklist for maintainers

  • 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
  • Make sure the JIRA ticket is resolved properly

@hbushouse hbushouse added this to the Build 10.2 milestone Mar 20, 2024
@hbushouse hbushouse changed the title Extend snowball core JP-3562: Extend snowball core Mar 20, 2024
@hbushouse
Copy link
Collaborator

Updates to the algorithms in stcal contained in spacetelescope/stcal#248

CHANGES.rst Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.23%. Comparing base (2fb073e) to head (cacbb30).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8303      +/-   ##
==========================================
- Coverage   75.31%   75.23%   -0.08%     
==========================================
  Files         474      474              
  Lines       38965    39000      +35     
==========================================
- Hits        29345    29343       -2     
- Misses       9620     9657      +37     
Flag Coverage Δ *Carryforward flag
nightly 77.33% <ø> (-0.01%) ⬇️ Carriedforward from 72e4dfa

*This pull request uses carry forward flags. Click here to find out more.

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

@zacharyburnett zacharyburnett mentioned this pull request Mar 21, 2024
21 tasks
@hbushouse
Copy link
Collaborator

hbushouse commented Mar 22, 2024

Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

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

Regression test failures with errors failed on setup with "IndexError: list index out of range" and stpipe.config_parser.ValidationError: Config parameter 'snowball_time_masked_next_int(default': missing were probably caused by the one typo in the spec string.

It looks like these account for all regtest failures except one, an assertion error in lib.tests.test_suffix.[stable-deps] test_suffix_existence . I didn't look deeply into what this test is for, but it would be good to look into that one a bit more

It also looks like some unit tests might need to be checked. There are some failing tests with TypeError: detect_jumps() got an unexpected keyword argument 'min_diffs_single_pass'

jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump.py Outdated Show resolved Hide resolved
jwst/jump/jump.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

Yet another regtest run started after fixing all the typos in the code: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1345

jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
@nden
Copy link
Collaborator

nden commented Mar 23, 2024

While looking at the spec I noticed use_ellipses has a comment it was deprecated. As far as I can tell it's not used in the stcal code any more. However, the deprecation is only visible to those that read comments on parameters. It needs to be deprecated in the code properly before being removed.

jwst/jump/jump_step.py Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

FINALLY got a complete regtest run at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1347/ which shows the same 2 failures as the most recent run against master and are unrelated. So this looks clean.

@hbushouse
Copy link
Collaborator

The continued failures in the CI tests are due to them using the released version of stcal, which doesn't yet have the corresponding updates needed by this PR.

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

I'm incorporating review comments about argument names and descriptions

docs/jwst/jump/arguments.rst Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

While looking at the spec I noticed use_ellipses has a comment it was deprecated. As far as I can tell it's not used in the stcal code any more. However, the deprecation is only visible to those that read comments on parameters. It needs to be deprecated in the code properly before being removed.

If it's OK with you, I'd like to merge this as is, now that code at least runs and the argument names have been cleaned up a bit. We can then do another PR to deprecate use_ellipses later.

jwst/jump/jump_step.py Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

Final regtest run at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1349/ Only 2 unrelated failures. This looks clean.

@hbushouse hbushouse enabled auto-merge (squash) March 24, 2024 18:16
@hbushouse hbushouse disabled auto-merge March 25, 2024 11:50
@hbushouse hbushouse merged commit 4d8c648 into spacetelescope:master Mar 25, 2024
21 of 29 checks passed
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.

None yet

4 participants