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

Specify number of cores for jump and ramp_fit #183

Merged
merged 13 commits into from
Aug 11, 2023

Conversation

mwregan2
Copy link
Collaborator

@mwregan2 mwregan2 commented Jul 25, 2023

This PR addresses the problem that for multiprocessing the number of cores to use can only be a fraction of the total number of cores. This is fine for small machines but on large clusters even 1/4 can be larger than someone is allowed to use.
The PR adds the flexibility to specify explicitly the number of cores the user wants to use. Changes were made for both Jump and Ramp fitting and new methods were written with unit tests.
No code changes are needed in the JWST repo only in the documentation.

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

For some users on large machines even using 'quarter' leads to them using too many cores. This change allows string integer values to be used.
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Patch coverage: 98.59% and project coverage change: +1.24% 🎉

Comparison is base (0c7cb32) 74.11% compared to head (72a7e83) 75.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
+ Coverage   74.11%   75.36%   +1.24%     
==========================================
  Files          33       33              
  Lines        6100     6039      -61     
==========================================
+ Hits         4521     4551      +30     
+ Misses       1579     1488      -91     
Files Changed Coverage Δ
tests/test_twopoint_difference.py 100.00% <ø> (+8.56%) ⬆️
src/stcal/jump/jump.py 66.33% <93.33%> (+3.40%) ⬆️
src/stcal/ramp_fitting/gls_fit.py 64.39% <100.00%> (+0.16%) ⬆️
src/stcal/ramp_fitting/ols_fit.py 80.17% <100.00%> (+0.03%) ⬆️
src/stcal/ramp_fitting/utils.py 91.03% <100.00%> (+0.41%) ⬆️
tests/test_jump.py 94.60% <100.00%> (+6.14%) ⬆️
tests/test_ramp_fitting.py 89.57% <100.00%> (+0.23%) ⬆️

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

Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci left a comment

Choose a reason for hiding this comment

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

I'd like to request the ramp fit test be moved to a different location per my specific comment above. Other than that, everything looks good.

tests/test_ramp_fitting.py Outdated Show resolved Hide resolved
@hbushouse hbushouse changed the title Specify number of cores Specify number of cores for jump and ramp_fit Jul 28, 2023
@hbushouse
Copy link
Collaborator

The CI failures are caused by unit tests in the jwst/ramp_fitting package trying to load the now obsolete function compute_slices (it's been renamed in this PR). Does the jwst PR contain updates to fix this?

@hbushouse
Copy link
Collaborator

OK, this is looking better. Just need to address the 2 errors coming from the code style CI test (removed unused imports from test modules).

@hbushouse
Copy link
Collaborator

Still need to have the style check errors fixed before we can merge:

tests/test_jump.py:3:24: F401 [] astropy.io.fits imported but unused
tests/test_twopoint_difference.py:3:24: F401 [
] astropy.io.fits imported but unused

@hbushouse
Copy link
Collaborator

I've fixed the code style errors.

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.

Finally all looks good.

@hbushouse hbushouse merged commit 2256c48 into spacetelescope:main Aug 11, 2023
18 checks passed
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Aug 11, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Oct 11, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Oct 27, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Nov 7, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Nov 10, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Nov 10, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Nov 11, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Nov 15, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Nov 16, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Nov 21, 2023
WilliamJamieson added a commit to WilliamJamieson/stcal that referenced this pull request Feb 13, 2024
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.

3 participants