spacetelescope / stcal

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

Adjust style checks to align with jwst #295

Open braingram opened 4 weeks ago

braingram commented 4 weeks ago

This PR adjusts the style checks to roughly align with jwst (with a few adjustments for things like this package using cython).

For reference the ruff checks and pre-commit hooks for jwst are as follows: https://github.com/spacetelescope/jwst/blob/e5c73fa23d157f4aa65416b4070c2b838e978494/pyproject.toml#L254 https://github.com/spacetelescope/jwst/blob/dd76184e82c1e863bce22d02de093f9bfe12f16f/.github/workflows/ci.yml

This PR sets the checks in this repo to roughly align:

I applied auto-fixes in: https://github.com/spacetelescope/stcal/pull/295/commits/a85bd00a2ceb5c9f1a26ea5992e04afd28c7741f These are largely import sorting but I think it's worth a second set of eyes on these changes as I did notice one log message formatting change that likely indicates a bug in the log message (where it's currently not formatting the expected variable): https://github.com/spacetelescope/stcal/pull/295/commits/a85bd00a2ceb5c9f1a26ea5992e04afd28c7741f#diff-c04284467dcbe86296aa89b83fd434e0dde8a32d790d157a9588ba3c7d169484R264

I then did some manual fixes removing unused variables and a duplicate test in: https://github.com/spacetelescope/stcal/pull/295/commits/3c59de9a3fecca54438291c716a68e65e89939fa

There are still failures with this PR that I'm not sure how to address.


src/stcal/ramp_fitting/likely_algo_classes.py:289:24: F821 Undefined name `fit_ramps`
    |
287 |         n = alpha.shape[0]
288 |         z = np.zeros((len(cvec), len(countrates)))
289 |         result_low_a = fit_ramps(z, self, sig, countrateguess=countrates)
    |                        ^^^^^^^^^ F821
290 | 
291 |         # try to avoid problems with roundoff error
    |

src/stcal/ramp_fitting/likely_algo_classes.py:294:25: F821 Undefined name `fit_ramps`
    |
292 |         da_incr = da * (countrates[np.newaxis, :] + sig**2)
293 | 
294 |         result_high_a = fit_ramps(z, self, sig, countrateguess=countrates + da_incr)
    |                         ^^^^^^^^^ F821
295 |         # finite difference approximation to dw/da
    |

tests/test_ramp_fitting_likely_fit.py:613:34: F821 Undefined name `cube1`
    |
611 | def dbg_print_cube(cube, pix=(0, 0), label=None):
612 |     data, dq, vp, vr, err = cube
613 |     data1, dq1, vp1, vr1, err1 = cube1
    |                                  ^^^^^ F821
614 |     row, col = pix
615 |     nints = data1.shape[0]

2 are uses of an undefined fit_ramps in likely_algo_classes.py: https://github.com/spacetelescope/stcal/blob/787aa81a6719bb0c72be8972171c7f3f88c8adf0/src/stcal/ramp_fitting/likely_algo_classes.py#L289 These would crash if anything tried to execute that code.

The last one is the use of an undefined cube in dbg_print_cube (which is unused). Calling this function would crash.

Tasks

news fragment change types... - ``changes/.apichange.rst``: change to public API - ``changes/.bugfix.rst``: fixes an issue - ``changes/.general.rst``: infrastructure or miscellaneous change
codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.53%. Comparing base (60bd3b8) to head (60138a4). Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/ramp_fitting/ramp_fit_class.py 0.00% 4 Missing :warning:
src/stcal/ramp_fitting/likely_algo_classes.py 0.00% 1 Missing :warning:
tests/test_ramp_fitting_likely_fit.py 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #295 +/- ## ========================================== + Coverage 86.21% 86.53% +0.32% ========================================== Files 47 49 +2 Lines 8812 8826 +14 ========================================== + Hits 7597 7638 +41 + Misses 1215 1188 -27 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 4 weeks ago

@kmacdonald-stsci Any advice on how to handle the bugs revealed by these style checks?

Can dbg_print_cube be removed?

What is the expected definition of fit_ramps in likely_algo_classes.py?

kmacdonald-stsci commented 4 weeks ago

@kmacdonald-stsci Any advice on how to handle the bugs revealed by these style checks?

Can dbg_print_cube be removed?

What is the expected definition of fit_ramps in likely_algo_classes.py?

I have a few dbg_ functions that I use for development and debugging, but aren't used at any other time. I'd prefer not to remove those functions.

The fit_ramps is defined here:

https://github.com/spacetelescope/stcal/blob/787aa81a6719bb0c72be8972171c7f3f88c8adf0/src/stcal/ramp_fitting/likely_fit.py#L493

braingram commented 4 weeks ago

Thanks!

I have a few dbg_ functions that I use for development and debugging, but aren't used at any other time. I'd prefer not to remove those functions.

I added a noqa for this line. It will however continue to fail if used.

The fit_ramps is defined here:

https://github.com/spacetelescope/stcal/blob/787aa81a6719bb0c72be8972171c7f3f88c8adf0/src/stcal/ramp_fitting/likely_fit.py#L493

Since likely_fit imports likely_algo_classes: https://github.com/spacetelescope/stcal/blob/787aa81a6719bb0c72be8972171c7f3f88c8adf0/src/stcal/ramp_fitting/likely_fit.py#L16 I think the import of fit_ramps needs to be local to avoid a circular import error. I updated the PR to include the local import.

braingram commented 4 weeks ago

@kmacdonald-stsci any suggestions for a second reviewer? Most of the changes are code style but the duplicate jump test might be worth checking with a maintainer of that code to see if the duplicate is supposed to be different.

kmacdonald-stsci commented 4 weeks ago

@kmacdonald-stsci any suggestions for a second reviewer? Most of the changes are code style but the duplicate jump test might be worth checking with a maintainer of that code to see if the duplicate is supposed to be different.

For anything in STCAL, if you need an additional reviewer besides me, I think you can always tag @melanieclarke and/or @tapastro.

braingram commented 4 weeks ago

Thanks for the quick review @melanieclarke Let me know how the jwst checks evolve and we can update these as well.