spacetelescope / stcal

https://stcal.readthedocs.io/en/latest/
Other
10 stars 32 forks source link

Issue-168: Updating Usage of 'np.where' Function #169

Closed kmacdonald-stsci closed 1 year ago

kmacdonald-stsci commented 1 year ago

Closes #168

This PR addresses the usage of np.where and removes it where possible for performance reasons in the ramp fitting step.

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.31 :tada:

Comparison is base (a3c1e27) 74.78% compared to head (557ef77) 75.10%.

:exclamation: Current head 557ef77 differs from pull request most recent head 66b10ff. Consider uploading reports for the commit 66b10ff to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #169 +/- ## ========================================== + Coverage 74.78% 75.10% +0.31% ========================================== Files 29 29 Lines 5700 5664 -36 ========================================== - Hits 4263 4254 -9 + Misses 1437 1410 -27 ``` | [Impacted Files](https://app.codecov.io/gh/spacetelescope/stcal/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | | |---|---|---| | [src/stcal/ramp\_fitting/ols\_fit.py](https://app.codecov.io/gh/spacetelescope/stcal/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-c3JjL3N0Y2FsL3JhbXBfZml0dGluZy9vbHNfZml0LnB5) | `79.83% <100.00%> (+2.39%)` | :arrow_up: | | [src/stcal/ramp\_fitting/utils.py](https://app.codecov.io/gh/spacetelescope/stcal/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-c3JjL3N0Y2FsL3JhbXBfZml0dGluZy91dGlscy5weQ==) | `90.57% <100.00%> (-0.15%)` | :arrow_down: | | [tests/test\_ramp\_fitting.py](https://app.codecov.io/gh/spacetelescope/stcal/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-dGVzdHMvdGVzdF9yYW1wX2ZpdHRpbmcucHk=) | `88.09% <100.00%> (-0.24%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

kmacdonald-stsci commented 1 year ago

https://github.com/spacetelescope/stcal/blob/main/src/stcal/ramp_fitting/ols_fit.py#L1657-L1658

The added test was to increase code coverage and add a case that needs testing.

hbushouse commented 1 year ago

@PaulHuwe can you (or your designee) run a Roman regtest against this PR branch?

braingram commented 1 year ago

@PaulHuwe can you (or your designee) run a Roman regtest against this PR branch?

I think I set this up correctly to run the Roman devdeps regtest with the source branch for this PR: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-devdeps/295/

hbushouse commented 1 year ago

@PaulHuwe can you (or your designee) run a Roman regtest against this PR branch?

I think I set this up correctly to run the Roman devdeps regtest with the source branch for this PR: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-devdeps/295/

There were a number of failures, with only 1 in the ramp_fitting step itself. It appears to be an unrelated error that is common to all the failures. Should we call this a success?

braingram commented 1 year ago

There were a number of failures, with only 1 in the ramp_fitting step itself. It appears to be an unrelated error that is common to all the failures. Should we call this a success?

I would call this a success but @PaulHuwe might know more about the existing failures which all appear to be unit related.

The previous devdeps run (started by timer with no overrides) had 19 failures: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-devdeps/294/

The run that used the source branch for this PR had 15 failures. All of these appear to match failures from the previous run: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FRoman-devdeps/detail/Roman-devdeps/295/tests/

PaulHuwe commented 1 year ago

Not that is matters, as this was already merged, but the RomanCAL test repo files are presently being updated, as the latest release required remaking of files.