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

Migrate to Ruff #62

Merged
merged 56 commits into from
Sep 22, 2023
Merged

Migrate to Ruff #62

merged 56 commits into from
Sep 22, 2023

Conversation

marcofavoritobi
Copy link
Contributor

@marcofavoritobi marcofavoritobi commented Aug 24, 2023

Proposed changes

This PR includes Ruff linter in the development and CI workflows. It subsumes other linters such as flake8, pylint, isort etc. in a single and efficient tool.

The PR was structured so to apply the changes incrementally, aiming at guaranteeing the integrity of the intermediate states of the software, by handling one family of rules at a time. If the changes were too complex for a single family of rules, the changes were split into multiple commits.

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023

Codecov Report

Merging #62 (628399d) into main (4741ff8) will decrease coverage by 2.16%.
The diff coverage is 81.01%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
- Coverage   91.38%   89.22%   -2.16%     
==========================================
  Files          42       42              
  Lines        1729     1782      +53     
==========================================
+ Hits         1580     1590      +10     
- Misses        149      192      +43     
Files Changed Coverage Δ
black_it/samplers/random_uniform.py 100.00% <ø> (ø)
black_it/schedulers/rl/agents/base.py 83.33% <ø> (ø)
black_it/schedulers/rl/rl_scheduler.py 25.00% <25.00%> (-3.77%) ⬇️
black_it/utils/time_series.py 75.00% <54.54%> (+1.78%) ⬆️
black_it/schedulers/rl/envs/mab.py 41.17% <60.00%> (-2.58%) ⬇️
black_it/loss_functions/minkowski.py 86.66% <66.66%> (-13.34%) ⬇️
black_it/samplers/base.py 92.50% <66.66%> (-7.50%) ⬇️
black_it/schedulers/base.py 90.62% <66.66%> (-9.38%) ⬇️
black_it/schedulers/rl/agents/epsilon_greedy.py 35.48% <66.66%> (+5.18%) ⬆️
black_it/schedulers/rl/envs/base.py 56.41% <66.66%> (-0.35%) ⬇️
... and 23 more

@marcofavoritobi marcofavoritobi force-pushed the ruff branch 20 times, most recently from 31060a5 to 743d153 Compare August 30, 2023 15:31
@marcofavoritobi marcofavoritobi force-pushed the ruff branch 4 times, most recently from 35ca6c5 to 0440035 Compare September 1, 2023 10:45
@marcofavoritobi marcofavoritobi marked this pull request as ready for review September 1, 2023 10:47
To reproduce the errors:
```
ruff check --select "SLF" black_it tests examples scripts
```

Some errors were fixed automatically by using the flag --fix, while others manually (e.g. g.legend in place of g._legend in plot module).
To reproduce the errors:
```
ruff check --select "SIM" black_it tests examples scripts
```

All the errors were automatically fixed with --fix flag.
To reproduce the errors:
```
ruff check --select "TCH" black_it tests examples scripts
```

All the errors were fixed by using the --fix flag.
To reproduce the errors:
```
ruff check --select "ARG" black_it tests examples scripts
```

Errors were mostly fixed manually.
To reproduce the errors:
```
ruff check --select "PTH" black_it tests examples scripts
```

Errors were mostly fixed manually.
To reproduce the errors:
```
ruff check --select "TD" black_it tests examples scripts
```
To reproduce the errors:
```
ruff check --select "ERA" black_it tests examples scripts
```

We decided to ignore the errors in the examples/ code because they might be useful as inlined code documentation.
To reproduce the errors:
```
ruff check --select "PD" black_it tests examples scripts
```
To reproduce the errors:
```
ruff check --select "PGH" black_it tests examples scripts
```
To reproduce the errors:
```
ruff check --select "PLR2004" black_it tests examples scripts
```

We tried to not ignore errors as much as possible. This kind of error in the test files do not make much sense (expected values are usually "magic values" in code to make the tests more readable). Other errors were fixed by either replacing floats to ints (e.g. 0.0 -> 0, 1.0 -> 1), or moving the magic value to a module-level private constant.
To reproduce errors:
```
ruff check --select "PL" black_it tests examples scripts\n
```
To reproduce the errors:
```
ruff check --select "TRY" black_it tests examples scripts
```
To reproduce the above errors (the output will include also others):
```
ruff check --select "NPY" black_it tests examples scripts\n
```

In this commit, we explicitly ignored these errors:
- examples/models/economics/boltzmann_wealth.py:93, we kept np.random.binomial since would have required a change in the APIs. This could be addressed separately in another commit.
@marcofavoritobi marcofavoritobi force-pushed the ruff branch 2 times, most recently from 5e920ca to e3dccc3 Compare September 21, 2023 11:57
To reproduce the errors:
```
ruff check --select "NPY" examples/models/economics/brock_hommes.py
```

We also added tests to ensure reproducibility with fixed seed.
The fixes changed the behaviour of the models even if the same seed was fed before and after the changes.

To reproduce:
```
import numpy as np
from scipy.stats import alpha, bernoulli

np.random.seed(0)
print("NP.RANDOM")
print(bernoulli.rvs(0.5, size=1))
print(bernoulli.rvs(0.5, size=1))

print("RNG")
rng = np.random.default_rng(seed=0)
print(bernoulli.rvs(0.5, size=1, random_state=rng))
print(bernoulli.rvs(0.5, size=1, random_state=rng))
```

Output:
```
NP.RANDOM
[1]
[1]
RNG
[1]
[0]
```
Fixed NPY002 issues in SIR models implemented in examples/models/sir/sir_python.py.

Added tests to verify the new seed management works.
To reproduce the errors:
```
ruff check --select "NPY" tests
```

This commit updates the tests code so to make them to use np.random.default_rng with a certain seed, rather than relying on global random seed handling, i.e. using np.random.seed.

To do so, a new fixture, 'rng', has been added so to avoid the tests code to initialize a np.random.Generator instance manually.

This allowed to remove the special case for macOS platforms in TestCalibrate.test_calibrator_calibrate tests (but not for Windows).
To reproduce the errors:
```
ruff check --select "PERF" black_it tests examples scripts
```
To reproduce the changes:
```
ruff check --fix black_it tests examples scripts
make black
ruff check --fix black_it tests examples scripts
```
…_init__

A good practice is to use the coarsest type as argument function, so to make it usable in more ways.

In particular, in this commit, the argument coordinate_filters in the base constructor of the class BaseLoss has been changed from 'list' to 'Sequence'. This allows to use coordinate_filters as tuples. It is also a more correct type hint, since coordinate_filters should not change during the lifetime of BaseLoss.
@marcofavoritobi marcofavoritobi merged commit e523145 into main Sep 22, 2023
11 checks passed
@marcofavoritobi marcofavoritobi deleted the ruff branch September 22, 2023 08:31
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.

4 participants