8000 JP-3564: Rebase nrs deepcopy by nden · Pull Request #8793 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-3564: Rebase nrs deepcopy #8793

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
Merged
merged 22 commits into from
Sep 20, 2024
Merged

JP-3564: Rebase nrs deepcopy #8793

merged 22 commits into from
Sep 20, 2024

Conversation

nden
Copy link
Collaborator
@nden nden commented Sep 17, 2024 •
edited by melanieclarke
Loading

Resolves JP-3564

Closes #8338

This PR is a copy of #8587 with minor changes to the names of functions. Below is a copy of the comment in the original comment by Tim Brandt.

This PR addresses the long run times of calwebb_spec2 for NIRSpec data, primarily in IFU mode, due to deep copying of WCS objects. These deep copies can take about 70% of the run time because they take place within loops over the pseudo-slits. This PR implements helper functions that copy then necessary bits of the WCS objects and then apply them to a single copy, reducing the number of deep copies of full WCS objects by about a factor of 30 in NIRSpec IFU mode. The runtime of calwebb_spec2 for NIRSpec IFU data decreases by almost a factor of 3.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API?
    • add an entry to CHANGES.rst within the relevant release section (otherwise add the no-changelog-entry-needed label to this PR)
    • 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

@nden nden requested a review from a team as a code owner September 17, 2024 14:26
@nden nden marked this pull request as draft September 17, 2024 14:26
@codecov Codecov
Copy link
codecov bot commented Sep 17, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.62162% with 21 lines in your changes missing coverage. Please review.

Project coverage is 61.80%. Comparing base (da9691d) to head (2c21b8d).

Files with missing lines Patch % Lines
jwst/assign_wcs/nirspec.py 91.89% 3 Missing ⚠️
jwst/clean_flicker_noise/clean_flicker_noise.py 70.00% 3 Missing ⚠️
jwst/flatfield/flat_field.py 0.00% 3 Missing ⚠️
jwst/master_background/expand_to_2d.py 0.00% 3 Missing ⚠️
jwst/photom/photom.py 0.00% 3 Missing ⚠️
jwst/cube_build/blot_cube_build.py 0.00% 2 Missing ⚠️
jwst/pathloss/pathloss.py 0.00% 2 Missing ⚠️
jwst/pixel_replace/pixel_replace.py 33.33% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8793      +/-   ##
==========================================
+ Coverage   61.75%   61.80%   +0.04%     
==========================================
  Files         377      377              
  Lines       38750    38828      +78     
==========================================
+ Hits        23931    23997      +66     
- Misses      14819    14831      +12

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

@nden nden force-pushed the rebase-nrs-deepcopy branch from 7810add to e6adbb6 Compare September 17, 2024 15:33
@tapastro
Copy link
Contributor

Regtest run started after the okify work was completed: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1727/

@melanieclarke
Copy link
Collaborator

@nden - there were some changes in jwst/nsclean/nsclean.py in Tim's original PR that should be ported forward to their new location in jwst/clean_flicker_noise/clean_flicker_noise.py. Are you still working on this one? I can send a code suggestion for where to get that in if it would help.

@nden nden force-pushed the rebase-nrs-deepcopy branch from 792ab88 to 1826b7a Compare September 18, 2024 20:34
@nden
Copy link
Collaborator Author
nden commented Sep 18, 2024

@melanieclarke Yes, please point me to the nsclean code change and I'll include it here.

@melanieclarke
Copy link
Collaborator

The changes from Tim are these ones:
https://github.com/spacetelescope/jwst/pull/8587/files#diff-2f4a109d4d47eb6cd2e20de128cbce6fcba1bc6c255b765b8f4834bd1586476a

They should be ported in to the mask_ifu_slices and mask_slits functions in the clean_flicker_noise.py file, here, in your branch:
https://github.com/nden/jwst/blob/1826b7a954e955ecda66ba0deb33a67d700ce9a7/jwst/clean_flicker_noise/clean_flicker_noise.py#L143

I can't directly add a suggested change, but I can send a PR to your branch if that's not enough information.

@nden
Copy link
Collaborator Author
nden commented Sep 18, 2024

Thanks @melanieclarke. Made a commit with the changes.

@nden nden added this to the Build 11.1 milestone Sep 18, 2024
@nden nden marked this pull request as ready for review September 18, 2024 21:39
@nden
Copy link
Collaborator Author
nden commented Sep 19, 2024

Failures in the latest run are unrelated.
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1730/

melanieclarke
melanieclarke approved these changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator
@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

I compared this PR to Tim's original one I reviewed before, and all the changes look good.

The change log needs de-conflicting again, then this should be ready. Thanks!

@melanieclarke melanieclarke mentioned this pull request Sep 19, 2024
Closed
8 tasks
t-brandt and others added 15 commits September 19, 2024 20:56
@nden nden force-pushed the rebase-nrs-deepcopy branch from 3b7d052 to 2c21b8d Compare September 20, 2024 01:01
@nden
Copy link
Collaborator Author
nden commented Sep 20, 2024

Done

@tapastro
Copy link
Contributor

Most recent regtests show matching failures to main, so I'm merging. Thanks! https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1736/

@tapastro tapastro merged commit 9fdf2c1 into spacetelescope:master Sep 20, 2024
29 checks passed
@melanieclarke melanieclarke changed the title Rebase nrs deepcopy JP-3564: Rebase nrs deepcopy Sep 20, 2024
@stscijgbot-jp stscijgbot-jp mentioned this pull request Sep 20, 2024
Closed
@braingram braingram mentioned this pull request Sep 24, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melanieclarke melanieclarke melanieclarke approved these changes

Assignees
No one assigned
Labels
assign_wcs cube_build flatfield master_background msa_flagging NIRSPEC path loss photom
Projects
None yet
Milestone
Build 11.1
Development

Successfully merging this pull request may close these issues.

Modify NIRSpec IFU WCS to improve runtime and usability
4 participants
@nden @tapastro @melanieclarke @t-brandt
0