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

Modernize Python code with ruff, CI, pre-commit #85

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Modernize Python code with ruff, CI, pre-commit #85

merged 5 commits into from
Aug 1, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jul 9, 2024

Description

This follows Configuration-for-linting-and-formatting-with-ruff to bring the Python code in arc up to basic modern standards for format and lint checking.

This PR is most easily reviewed by looking at the individual commits. There should be very little that is controversial.

Interface impacts

None.

Functional testing on kady

Results at: https://icxc.cfa.harvard.edu/aspect/test_review_outputs/arc/pr85/arc3/

The output plots look fine and there were no processing errors.

In flight ska3 on kady:

$ cd ~/git/arc
$ git rev-parse HEAD
aa195faf36b35dac72414499e3d114d91fcbe41f

$ mkdir -p ~/ska/www/ASPECT_ICXC/test_review_outputs/arc/pr85/arc3
$ ln -s /proj/sot/ska/www/ASPECT_ICXC/test_review_outputs/arc/pr85/arc3 ./arc3

$ cp ${SKA}/data/arc3/*.h5 arc3/
$ cp ${SKA}/data/arc3/*.dat arc3/

$ ./get_goes_x.py --h5 arc3/GOES_X.h5 
$ ./plot_goes_x.py --h5 arc3/GOES_X.h5 --out=arc3/goes_x.png
$ ./get_ace.py --h5 arc3/ACE.h5 
$ ./get_hrc.py --h5 arc3/hrc_shield.h5 --data-dir arc3
$ ./make_timeline.py --data-dir arc3

$ lst arc3/
ska3-kady$ lst arc3/
total 331940
lrwxrwxrwx 1 aldcroft aldcroft        49 Aug  1 07:49 ACE_hourly_avg.npy -> /proj/sot/ska3/flight/data/arc/ACE_hourly_avg.npy
-rw-rw-r-- 1 aldcroft aldcroft     20656 Aug  1 07:50 web_content.dat
-rw-rw-r-- 1 aldcroft aldcroft  44544304 Aug  1 07:57 GOES_X.h5
-rw-rw-r-- 1 aldcroft aldcroft     53268 Aug  1 07:57 goes_x.png
-rw-rw-r-- 1 aldcroft aldcroft  35534625 Aug  1 07:57 ACE.h5
-rw-rw-r-- 1 aldcroft aldcroft 258116176 Aug  1 07:58 hrc_shield.h5
-rw-rw-r-- 1 aldcroft aldcroft        32 Aug  1 07:58 hrc_shield.dat
-rw-rw-r-- 1 aldcroft aldcroft        32 Aug  1 07:58 p4gm.dat
-rw-rw-r-- 1 aldcroft aldcroft        34 Aug  1 07:58 p41gm.dat
-rw-rw-r-- 1 aldcroft aldcroft     71777 Aug  1 07:58 timeline.png
-rw-rw-r-- 1 aldcroft aldcroft    183614 Aug  1 07:58 timeline_states.js

@taldcroft taldcroft requested review from jeanconn and removed request for jeanconn July 15, 2024 20:33
Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, did you want me to do the functional testing by running a test arc before doing a review?

@taldcroft
Copy link
Member Author

@jeanconn - I did the functional testing and documented it in the description.

print(hrc_shield[ok].mean(), times[ok].mean(), file=f)

# For GOES earlier than 16:
# for colname, scale, filename in zip(
# ('p2', 'p5'), (3.3, 12.0), ('p4gm.dat', 'p41gm.dat')):
# GOES-16, ``scale`` TBD
for colname, scale, filename in zip(
('p4', 'p7'), (3.3, 12.0), ('p4gm.dat', 'p41gm.dat')):
("p4", "p7"), (3.3, 12.0), ("p4gm.dat", "p41gm.dat"), strict=False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a little odd to be explicit about strict=False, but that matches the default behavior so that's fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a ruff thing, trying to highlight that the default for zip is prone to subtle bugs. In this case it is a bit silly but harmless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I knew this was a ruff thing in context - though I wondered if it would make sense to just use strict=True in these use cases if one is going to bother to add the strict keyword.

@taldcroft taldcroft merged commit 905cb18 into master Aug 1, 2024
2 checks passed
@taldcroft taldcroft deleted the ruff branch August 1, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants