Ruff #184

jeanconn · 2024-11-29T15:17:32Z

Description

Ruff

Interface impacts

Testing

Unit tests

Mac

(ska3-flight-latest) flame:chandra_aca jean$ git rev-parse HEAD
5ebd0e3e2253e486a9147698eed047caff7f55e1
(ska3-flight-latest) flame:chandra_aca jean$ pytest
===================================================== test session starts ======================================================
platform darwin -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
PyQt5 5.15.9 -- Qt runtime 5.15.8 -- Qt compiled 5.15.8
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: astropy-0.11.0, qt-4.4.0, cov-5.0.0, timeout-2.2.0, remotedata-0.4.1, anyio-4.3.0, filter-subpackage-0.2.0, doctestplus-1.2.1, astropy-header-0.2.2, hypothesis-6.112.0, arraydiff-0.6.1, mock-3.14.0
collected 230 items                                                                                                            

chandra_aca/tests/test_aca_image.py ..................                                                                   [  7%]
chandra_aca/tests/test_all.py ........................                                                                   [ 18%]
chandra_aca/tests/test_attitude.py .............................................................                         [ 44%]
chandra_aca/tests/test_dark_model.py ............                                                                        [ 50%]
chandra_aca/tests/test_dark_subtract.py ........                                                                         [ 53%]
chandra_aca/tests/test_drift.py ..........................                                                               [ 64%]
chandra_aca/tests/test_maude_decom.py .........................                                                          [ 75%]
chandra_aca/tests/test_planets.py ...............                                                                        [ 82%]
chandra_aca/tests/test_psf.py ...                                                                                        [ 83%]
chandra_aca/tests/test_residuals.py ss...                                                                                [ 85%]
chandra_aca/tests/test_star_probs.py .................................                                                   [100%]

======================================================= warnings summary =======================================================
chandra_aca/chandra_aca/tests/test_residuals.py::test_obc
  /Users/jean/miniforge3/envs/ska3-flight-latest/lib/python3.11/site-packages/django/utils/encoding.py:266: DeprecationWarning: 'locale.getdefaultlocale' is deprecated and slated for removal in Python 3.15. Use setlocale(), getencoding() and getlocale() instead.
    encoding = locale.getdefaultlocale()[1] or 'ascii'

chandra_aca/chandra_aca/tests/test_residuals.py::test_obc
  /Users/jean/miniforge3/envs/ska3-flight-latest/lib/python3.11/site-packages/django/http/request.py:1: DeprecationWarning: 'cgi' is deprecated and slated for removal in Python 3.13
    import cgi

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
==================================== 228 passed, 2 skipped, 2 warnings in 70.93s (0:01:10)

Independent check of unit tests by [REVIEWER NAME]

[PLATFORM]:

Functional tests

No functional testing.

Copilot reviewed 15 out of 30 changed files in this pull request and generated no suggestions.

Files not reviewed (15)

.github/workflows/black.yml: Language not supported
.github/workflows/flake8.yml: Language not supported
chandra_aca/centroid_resid.py: Evaluated as low risk
chandra_aca/darkbins.py: Evaluated as low risk
chandra_aca/dark_model.py: Evaluated as low risk
chandra_aca/attitude.py: Evaluated as low risk
chandra_aca/tests/test_attitude.py: Evaluated as low risk
chandra_aca/drift.py: Evaluated as low risk
chandra_aca/aca_image.py: Evaluated as low risk
chandra_aca/tests/test_aca_image.py: Evaluated as low risk
chandra_aca/tests/test_all.py: Evaluated as low risk
chandra_aca/plot.py: Evaluated as low risk
chandra_aca/planets.py: Evaluated as low risk
chandra_aca/tests/test_dark_model.py: Evaluated as low risk
chandra_aca/star_probs.py: Evaluated as low risk

taldcroft

Looks mostly good, thanks!

chandra_aca/maude_decom.py

chandra_aca/plot.py

chandra_aca/star_probs.py

chandra_aca/tests/test_aca_image.py

jeanconn · 2024-11-29T17:03:22Z

PS I hadn't asked for review yet until #174 is reviewed and merged.

taldcroft · 2024-11-29T19:21:02Z

PS I hadn't asked for review yet until #174 is reviewed and merged.

Oops, I guess I just got excited. And ruff review is about where I'm at today.

jeanconn · 2024-12-05T20:46:58Z

I think this is ready for review. I tried to separate the auto ruff pieces from my manual fixes in the separate commits.

Copilot reviewed 15 out of 30 changed files in this pull request and generated no suggestions.

Files not reviewed (15)

.github/workflows/black.yml: Language not supported
.github/workflows/flake8.yml: Language not supported
chandra_aca/darkbins.py: Evaluated as low risk
chandra_aca/attitude.py: Evaluated as low risk
chandra_aca/dark_model.py: Evaluated as low risk
chandra_aca/tests/test_attitude.py: Evaluated as low risk
chandra_aca/drift.py: Evaluated as low risk
chandra_aca/aca_image.py: Evaluated as low risk
chandra_aca/tests/test_all.py: Evaluated as low risk
chandra_aca/tests/test_aca_image.py: Evaluated as low risk
chandra_aca/planets.py: Evaluated as low risk
chandra_aca/plot.py: Evaluated as low risk
chandra_aca/centroid_resid.py: Evaluated as low risk
chandra_aca/tests/test_dark_model.py: Evaluated as low risk
chandra_aca/star_probs.py: Evaluated as low risk

taldcroft

This is all looks good. Thanks for the way you separated all the by-hand changes, this made the review much easier!

I made the same comment twice. Both versions you put in will give the right answer, so I'm OK with just merging this as-is.

chandra_aca/maude_decom.py

taldcroft · 2024-12-05T21:08:57Z

chandra_aca/maude_decom.py

-    if blobs and frames:
-        raise ValueError("Specify only one of 'blobs' or 'frames'")
+    if (not blobs and not frames) or (frames and blobs):
+        raise ValueError("Specify one and only one of 'blobs' or 'frames'")


Repeating the comment, basically that the version in 9d0a53 was actually correct.

OK, the original logic was unnecessarily complex. It was

assert frames XOR blobs # one and only one of frame/blobs is set

However, the statement before ensures that at least one of them is always set, so then here (indeed) you can just check blobs and frames.

Maybe it would be worth a documentation update to reflect that if neither is set, then frames is the default.

I made the other comment on code that got changed, so it might not show up.

I see everything!

jeanconn · 2024-12-05T21:24:10Z

Regarding "so I'm OK with just merging this as-is." Thanks! I think it was a good point in your original review that the change wasn't logically equivalent. It was a process mistake on my part to try to simplify that logic a bit but not call appropriate attention to it in review. I think at least the change has had appropriate review (thanks!) and if we want to do some more simplification, at least it is starting with passing ruff!

jeanconn requested a review from Copilot November 29, 2024 15:46

Copilot AI reviewed Nov 29, 2024

View reviewed changes

taldcroft requested changes Nov 29, 2024

View reviewed changes

chandra_aca/maude_decom.py Outdated Show resolved Hide resolved

chandra_aca/plot.py Show resolved Hide resolved

chandra_aca/star_probs.py Outdated Show resolved Hide resolved

chandra_aca/tests/test_aca_image.py Show resolved Hide resolved

jeanconn changed the title ~~Ruff~~ WIP: Ruff Nov 29, 2024

Base automatically changed from back-subtract to master December 3, 2024 16:41

jeanconn added 17 commits December 3, 2024 14:28

Set for ruff

65c704a

Ruff format (auto)

9b9d6eb

Ruff check --fix

aa9bb26

Docs fixes

d15dfd5

Exclude borrowed cubic_spline code from ruff

e06d953

Remove noqa not needed

f7237af

Remove some asserts

9d0a53f

Replace relative imports

b973e33

Fix some issues with variable redefinition

c0424dc

Remove some not-used enumerates

46dd898

Add some noqa

fa32223

Add some list comps and replace dict with literal

f1b0ebf

Simplify check on fetch_source

b784966

Rename unused loop vars with _

89fe55c

Fix grid option not actually used

1cba025

Raise error from None

fbb70ef

Update pre-commit

ebf103a

jeanconn force-pushed the ruff branch from 9aea615 to ebf103a Compare December 3, 2024 19:32

jeanconn added 2 commits December 3, 2024 14:48

Rename some loop variables

c0d8b16

Update logic for blobs frames input check

b7a3283

jeanconn changed the title ~~WIP: Ruff~~ Ruff Dec 5, 2024

jeanconn requested review from Copilot and taldcroft December 5, 2024 20:46

Copilot AI reviewed Dec 5, 2024

View reviewed changes

Rename wt variable

5ebd0e3

taldcroft approved these changes Dec 5, 2024

View reviewed changes

jeanconn merged commit 10aaa76 into master Dec 5, 2024
2 checks passed

jeanconn deleted the ruff branch December 5, 2024 21:24

javierggt mentioned this pull request Dec 9, 2024

2024.12 sot/skare3#1450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ruff #184

Ruff #184

jeanconn commented Nov 29, 2024 •

edited

Loading

taldcroft left a comment

jeanconn commented Nov 29, 2024

taldcroft commented Nov 29, 2024

jeanconn commented Dec 5, 2024

taldcroft left a comment

taldcroft Dec 5, 2024

taldcroft Dec 5, 2024

jeanconn Dec 5, 2024

jeanconn commented Dec 5, 2024

Ruff #184

Ruff #184

Conversation

jeanconn commented Nov 29, 2024 • edited Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

Choose a reason for hiding this comment

taldcroft left a comment

Choose a reason for hiding this comment

jeanconn commented Nov 29, 2024

taldcroft commented Nov 29, 2024

jeanconn commented Dec 5, 2024

Choose a reason for hiding this comment

taldcroft left a comment

Choose a reason for hiding this comment

taldcroft Dec 5, 2024

Choose a reason for hiding this comment

taldcroft Dec 5, 2024

Choose a reason for hiding this comment

jeanconn Dec 5, 2024

Choose a reason for hiding this comment

jeanconn commented Dec 5, 2024

jeanconn commented Nov 29, 2024 •

edited

Loading