Run automated formatting and linters #187

WilliamJamieson · 2023-08-11T18:29:41Z

This PR adds automated linters and code formatters to stcal.

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)

codecov · 2023-08-11T18:32:07Z

Codecov Report

Attention: 238 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
src/stcal/__init__.py	`100.00% <100.00%> (ø)`
src/stcal/alignment/resample_utils.py	`100.00% <100.00%> (ø)`
src/stcal/linearity/linearity.py	`100.00% <100.00%> (ø)`
src/stcal/ramp_fitting/ols_cas22/__init__.py	`100.00% <100.00%> (ø)`
src/stcal/ramp_fitting/ramp_fit_class.py	`55.91% <100.00%> (ø)`
src/stcal/saturation/saturation.py	`98.33% <100.00%> (-0.03%)`	⬇️
tests/test_alignment.py	`100.00% <100.00%> (ø)`
tests/test_dark_current.py	`100.00% <100.00%> (ø)`
tests/test_dq.py	`92.30% <100.00%> (ø)`
tests/test_jump.py	`95.02% <100.00%> (ø)`
... and 22 more

📢 Thoughts on this report? Let us know!

zacharyburnett

LGTM, we can also activate pre-commit.ci for this repo to automatically lint / format PRs, pending maintainer approval

Include all the base changes

Note the check-style job can be removed once the pre-commit.ci bot is enabled

WilliamJamieson · 2023-11-11T04:32:23Z

JWST passes regression tests: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1052/

kmacdonald-stsci

All in all it looks fine. I'm confused by some of the choices made and some of the syntax. Also, the PR should list what automated tools were used for formatting and linting. That way any future PR's can use those same tools to ensure all future changes conform to the current formatting and syntax.

kmacdonald-stsci · 2023-11-14T12:06:29Z

src/stcal/alignment/util.py

-    rotation: float = None,
-    transform=None,
-):
+    pscale_ratio: int | None = None,


Should the = None be removed from these?

This is correct. The | is notation introduced officially by Python 3.10 but available in Python 3.9 via from __future__ import annotations; is equivalent to using a typing.Union (e.g. a | b is equivalent to typing.Union[a, b]). Note that the typing.Union[a, b] annotation is saying for Python that the type is either a or b. It is not the equivalent of a C union type, that is more akin to the Python tuple, e.g. annotated by tuple[a, b].

As for the addition of the | None = None, the annotation/assignment foo: int=None is technically incorrect and will fail static type checkers, both built into IDEs for syntax highlighting and completion and strong checkers like MyPy. Instead it needs to be turned into an optional, foo: typing.Optional[int] = None which is short for foo: typing.Union[int, None] = None which as stated above can be reduced to the | symbol.

Essentially, what has happened here is that ruff noted the lack of the optional note in those annotations which clearly are optional and swapped that to using the optional annotation. Then the pyupgrade tool shortened it to using | None, which is the preferred way of writing optional annotations now according to the majority of style guides.

I like this new style, much more explicit but less characters at the same time

kmacdonald-stsci · 2023-11-14T12:11:27Z

src/stcal/alignment/util.py

-    fiducial: Union[tuple, np.ndarray],
-    disp_axis: int = None,
-    pscale_ratio: float = None,
+    fiducial: tuple | np.ndarray,


This was originally listed as a Union, now is not. Is that correct?

kmacdonald-stsci · 2023-11-14T12:11:47Z

src/stcal/alignment/util.py

-    disp_axis: int = None,
-    pscale_ratio: float = None,
+    fiducial: tuple | np.ndarray,
+    disp_axis: int | None = None,


Should the = None b here?

kmacdonald-stsci · 2023-11-14T12:19:17Z

src/stcal/alignment/util.py

-    def _reproject(
-        x: Union[float, np.ndarray], y: Union[float, np.ndarray]
-    ) -> tuple:
+    def _reproject(x: float | np.ndarray, y: float | np.ndarray) -> tuple:


Is it appropriate to remove Union here?

I think Python 3.10 changed Union to |; can this run on 3.9 with a future import?

from __future__ import annotations makes this work in Python 3.9 as this syntax was "trialed" in Python 3.9 before becoming official in Python 3.10.

kmacdonald-stsci · 2023-11-14T12:38:15Z

src/stcal/dark_current/dark_sub.py

@@ -86,24 +86,28 @@ def do_correction_data(science_data, dark_data, dark_output=None):
    drk_groupgap = dark_data.exp_groupgap

    log.info(
-        'Science data nints=%d, ngroups=%d, nframes=%d, groupgap=%d',


I think this was better in the old format. Also, why not use f-strings?

This is the fault of the auto-formatter (both black and ruff will do the same thing), which can be turned off locally if one wants to, but I didn't review the outputs for when it did strange things.

As for the f-string, this is due to a quirk in how python.logging actually functions under the hood.

When you log a message say log.debug(msg) whatever is in msg will be computed no matter what (e.g. if its an f-string or str.format string), meaning that even if the logging level is turned down so that in this case debug messages are not logged, there is still overhead from logging due to it still computing msg. However, fprintf string formatting can be written in a way that avoids this because python.logging passes all the arguments after the message string to the string using %, i.e. log.debug("my message with %s and %s", "this", "that)
will ultimately be rendered as "my message %s and %s" % ("this", "that") if the debug level logging is set, otherwise no rendering will occur.

There are other reasons why fprintf style string formatting is preferred by python.logging in addition to this. However, in any case the documentation for python.logging specifically says that the fprintf style strings should be preferred https://docs.python.org/3.12/howto/logging.html#logging-variable-data. This unfortunately is unlikely to change due to backwards compatiblity issues.

The why is this bad? section of https://docs.astral.sh/ruff/rules/logging-string-format/#why-is-this-bad goes into further details.

kmacdonald-stsci · 2023-11-14T12:53:39Z

src/stcal/ramp_fitting/gls_fit.py

@@ -105,22 +105,31 @@ def gls_ramp_fit(ramp_data, buffsize, save_opt, readnoise_2d, gain_2d, max_cores
    num_available_cores = cpu_count()
    number_slices = utils.compute_num_slices(max_cores, nrows, num_available_cores)

-    log.info(f"Number of data slices: {number_slices}")
+    log.info("Number of data slices: %s", number_slices)


Why does this remove the use of f-strings?

kmacdonald-stsci · 2023-11-14T13:06:47Z

src/stcal/ramp_fitting/ols_fit.py

@@ -159,10 +155,10 @@ def ols_ramp_fit_multiprocessing(
    opt_info: tuple
        The tuple of computed optional results arrays for fitting.
    """
-    log.info(f"Number of processors used for multiprocessing: {number_slices}")
+    log.info("Number of processors used for multiprocessing: %s", number_slices)


Why the removal of f-string?

github-actions bot added documentation Improvements or additions to documentation ramp_fitting automation testing jump linearity dark_current saturation installation labels Aug 11, 2023

WilliamJamieson force-pushed the feature/code_formatting branch from 23bfff3 to afe58c1 Compare August 11, 2023 18:38

zacharyburnett approved these changes Aug 14, 2023

View reviewed changes

WilliamJamieson added 18 commits November 10, 2023 20:03

Add pre-commit file

aed5fe1

Include all the base changes

Fix pygrep

a31d88f

Add ruff to pre-commit

4b07900

Apply ruff formatter

1c2ee28

Enable import sort linting

11578a9

Use the Ruff rules

4910bc8

Add logging string linting

dbee386

Use pyupgrade linting

57fc7a1

Add numpy linting

13589e8

Enable bandit linting

c977670

Add bugbear linting

4d816ab

Add builtin name shadowing linting

7b55b92

Add implicit string concat linting

317496d

Add misc improvement linting

a73d794

Add pytest linting

3e1f3fe

Add return statement linting

df57979

Add private member access linting

3c94610

Add pathlib linting

9e6ed4b

WilliamJamieson added 16 commits November 10, 2023 20:03

Add pygrep checks

b2aa1d6

Add pylint checks

8eff558

Add performance linting

1b0b9e1

Add codespell

2e2d783

Add repo review

b2d7d63

Add cython lint

e287933

Enable isort for cython files only

f0a8ea3

Clean up pyproject.toml file

1bbbc88

Enable prettier for non-python code formatting

27ff844

Enable some pydocstyle checks

059ee11

Add type annotation linting

afb839d

Enable error message linting

24822c1

Add simplification linting

e91b86e

Add type checking linting

5206b0f

Enable tryceratops linting

32bd6bb

Update CI

4b8fcca

Note the check-style job can be removed once the pre-commit.ci bot is enabled

WilliamJamieson force-pushed the feature/code_formatting branch from cd7518e to 4b8fcca Compare November 11, 2023 02:12

Update changes

2ce4799

Add soctests marker for romancal tests

108608b

WilliamJamieson marked this pull request as ready for review November 11, 2023 14:36

WilliamJamieson requested a review from a team as a code owner November 11, 2023 14:36

kmacdonald-stsci approved these changes Nov 14, 2023

View reviewed changes

WilliamJamieson merged commit 36d9d14 into spacetelescope:main Nov 14, 2023
22 of 23 checks passed

WilliamJamieson deleted the feature/code_formatting branch November 14, 2023 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run automated formatting and linters #187

Run automated formatting and linters #187

WilliamJamieson commented Aug 11, 2023

codecov bot commented Aug 11, 2023 •

edited

Loading

zacharyburnett left a comment •

edited

Loading

WilliamJamieson commented Nov 11, 2023

kmacdonald-stsci left a comment

kmacdonald-stsci Nov 14, 2023

WilliamJamieson Nov 14, 2023 •

edited

Loading

zacharyburnett Nov 14, 2023 •

edited

Loading

kmacdonald-stsci Nov 14, 2023

kmacdonald-stsci Nov 14, 2023

kmacdonald-stsci Nov 14, 2023

zacharyburnett Nov 14, 2023

WilliamJamieson Nov 14, 2023

kmacdonald-stsci Nov 14, 2023

WilliamJamieson Nov 14, 2023

WilliamJamieson Nov 14, 2023

kmacdonald-stsci Nov 14, 2023

kmacdonald-stsci Nov 14, 2023

Run automated formatting and linters #187

Run automated formatting and linters #187

Conversation

WilliamJamieson commented Aug 11, 2023

codecov bot commented Aug 11, 2023 • edited Loading

Codecov Report

zacharyburnett left a comment • edited Loading

Choose a reason for hiding this comment

WilliamJamieson commented Nov 11, 2023

kmacdonald-stsci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamJamieson Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

zacharyburnett Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 11, 2023 •

edited

Loading

zacharyburnett left a comment •

edited

Loading

WilliamJamieson Nov 14, 2023 •

edited

Loading

zacharyburnett Nov 14, 2023 •

edited

Loading