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-2546: updates and fixes for wfss_contam step #8417

Closed
wants to merge 14 commits into from

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Apr 10, 2024

Resolves JP-2546

Closes #7288

This PR addresses several of the remaining tasks needed to make the wfss_contam step fully functional. Submitting a draft now so everyone can see the proposed changes, although there are still several outstanding issues. See the JIRA ticket for many more details.

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

Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 79.92278% with 52 lines in your changes are missing coverage. Please review.

Project coverage is 75.72%. Comparing base (6580914) to head (632fdec).
Report is 4 commits behind head on master.

❗ Current head 632fdec differs from pull request most recent head 5efdbb6. Consider uploading reports for the commit 5efdbb6 to get more accurate results

Files Patch % Lines
jwst/wfss_contam/wfss_contam.py 68.46% 41 Missing ⚠️
jwst/wfss_contam/observations.py 91.57% 8 Missing ⚠️
jwst/wfss_contam/wfss_contam_step.py 25.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8417       +/-   ##
===========================================
+ Coverage   56.38%   75.72%   +19.34%     
===========================================
  Files         387      476       +89     
  Lines       38716    39661      +945     
===========================================
+ Hits        21830    30035     +8205     
+ Misses      16886     9626     -7260     
Flag Coverage Δ *Carryforward flag
nightly 77.74% <ø> (?) Carriedforward from 1859827

*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.

@emolter emolter closed this Jan 29, 2025
@emolter emolter deleted the JP-2546 branch January 29, 2025 23:12
@Rplesha
Copy link

Rplesha commented Feb 21, 2025

@emolter is this being worked in a different branch now, or was it decided the fixes would go a different direction? The last I heard @Russell-Ryan was going to test this out, so I was surprised to see it closed.

@emolter
Copy link
Collaborator Author

emolter commented Feb 24, 2025

Hi @Rplesha, I don't recall what the impetus was for closing the PR, aside from generally decreasing the number of stale drafts we have sitting around.

I wasn't aware that anyone was going to test this imminently, but now that I know it's a priority I'll start working on it again. The branch has become quite out-of-date with the latest changes elsewhere in the pipeline, and it may take a bit of work to get this back to being ready for review.

Many of the changes here are still (IMO) a good idea. I haven't looked at this in several months, but it appears that I had several questions for y'all on the JIRA ticket that ask about desired behaviors. It might be good for me and @Russell-Ryan to talk about this face-to-face on Slack or something to come up with a plan of action

@Rplesha
Copy link

Rplesha commented Feb 24, 2025

@emolter I believe we're thinking about it for build 12.0, but we're thinking about several updates for WFSS, so just to clarify I'm not actually sure where this falls in the priority right now. We also think there is a lot of good work here that we wanted to start from, which is I think why @Russell-Ryan was going to test it before we decided how else we wanted to proceed.

I had also tried running it out of curiosity on some data I'm investigating that is clearly contaminated some, and was running into a string of errors trying to get it to run on some data, so I can confirm there is some work needed to get it up and running again.

I believe that @drlaw1558 and Russell are planning to meet to come up with an overall WFSS pipeline plan, too, so I just wanted to loop David into this, too.

@emolter
Copy link
Collaborator Author

emolter commented Feb 24, 2025

Sounds good, thanks for looking at this. I'll let you know when this is running again, and you can test it on your data. I should have time to work on it this sprint.

@tapastro
Copy link
Contributor

@emolter The ticket is effectively on hold as WFSS reps determine the desired changes and coordinate those changes with David. In an ideal world, this ticket would have been closed after this PR was tested and merged, but this ticket now sits in a grey area; a new WFSS coordination group was recently formed to better inform SCSB on desired pipeline updates, and the group-determined update list may or may not match the work you've done here. It would be helpful to have these changes in a testable state for INS, though. I'll forward the invite to the next meeting - once the INS reps can test your changes on a new PR/branch, it will be useful to gather some feedback.

@drlaw1558
Copy link
Collaborator

I concur with @tapastro ; if you can update this so that it's compatible with the latest pipeline it would be helpful, but otherwise let's hold off on further development for now as this is part of a broader conversation about where to go with WFSS.

@emolter
Copy link
Collaborator Author

emolter commented Feb 24, 2025

Cool yeah that was already my plan

@emolter
Copy link
Collaborator Author

emolter commented Feb 24, 2025

@Rplesha @drlaw1558 @Russell-Ryan please see #9220 which will supersede this PR. (it was easier to just start a new PR, and we aren't losing much comment history anyway)

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.

Updates and fixes for WFSS contamination correction
4 participants