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

Adjust style checks to align with jwst #295

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Oct 2, 2024

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:

  • use default ruff rules
  • extend ruff rules to include isort (which was previously a pre-commit hook in this repo)
  • run bandit (like jwst does)
  • enable cython-lint (as this package uses cython)

I applied auto-fixes in:
a85bd00
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):
a85bd00#diff-c04284467dcbe86296aa89b83fd434e0dde8a32d790d157a9588ba3c7d169484R264

I then did some manual fixes removing unused variables and a duplicate test in:
3c59de9

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:

result_low_a = fit_ramps(z, self, sig, countrateguess=countrates)

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

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

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

Project coverage is 85.43%. Comparing base (5f94030) to head (84dbac6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/ramp_fitting/ramp_fit_class.py 0.00% 4 Missing ⚠️
src/stcal/ramp_fitting/likely_algo_classes.py 0.00% 1 Missing ⚠️
tests/test_ramp_fitting_likely_fit.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
+ Coverage   85.19%   85.43%   +0.23%     
==========================================
  Files          46       46              
  Lines        8804     8760      -44     
==========================================
- Hits         7501     7484      -17     
+ Misses       1303     1276      -27     

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

@braingram
Copy link
Collaborator Author

@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
Copy link
Collaborator

@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:

@braingram
Copy link
Collaborator Author

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:

Since likely_fit imports likely_algo_classes:

from .likely_algo_classes import IntegInfo, RampResult, Covar

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 braingram marked this pull request as ready for review October 2, 2024 13:22
@braingram braingram requested a review from a team as a code owner October 2, 2024 13:22
@braingram
Copy link
Collaborator Author

@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
Copy link
Collaborator

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

Copy link
Contributor

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

One lingering fix, for the f-string in the log message, but otherwise this looks good.

Thanks for doing this clean up work!

src/stcal/ramp_fitting/ramp_fit.py Show resolved Hide resolved
tests/test_jump.py Show resolved Hide resolved
@braingram
Copy link
Collaborator Author

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

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