-
Notifications
You must be signed in to change notification settings - Fork 102
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
UPD: change configuration project #363
Conversation
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
- sphinx=4.3.2 | ||
- sphinx-gallery=0.10.1 | ||
- sphinx_rtd_theme=1.0.0 | ||
- typing_extensions=4.0.1 | ||
- pandas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you remove the versions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because there's no incompatibility with the latest version of pandas so far. so there's no reason to constrain the version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still feel like we should force a version. We don't want the doc not building for a version problem. (we can perform version updates every now and then?)
|
||
zeros_scores_proba_last = (y_pred_proba_last <= EPSILON) | ||
zeros_scores_proba_last = y_pred_proba_last <= EPSILON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is easier to understand with the '()'
).filled(fill_value=np.inf), | ||
axis=1 | ||
), axis=1 | ||
np.min(np.ma.masked_less(y_pred_proba, EPSILON).filled(fill_value=np.inf), axis=1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example where I think it's less readable after black : here you have 4 numpy functions in 3 lines (expand_dims, min, masked_less, filled), quite hard to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I should have added pre-commit first, then thought about black.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally a very clean, good update. Thank you @thibaultcordier.
I think black has some disadvantages, but mainly, it will help clean up the code more than it will make it less readable.
I do wonder whether we should perform the linting before black or not. Please checkout comment.
- sphinx=4.3.2 | ||
- sphinx-gallery=0.10.1 | ||
- sphinx_rtd_theme=1.0.0 | ||
- typing_extensions=4.0.1 | ||
- pandas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still feel like we should force a version. We don't want the doc not building for a version problem. (we can perform version updates every now and then?)
np.random.multivariate_normal(center, cov, n_samples) | ||
for center, cov in zip(centers, covs) | ||
] | ||
[np.random.multivariate_normal(center, cov, n_samples) for center, cov in zip(centers, covs)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is very code reading friendly. Is this black
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is black
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, sometimes, black is really not friendly. 😆
@@ -1,7 +1,10 @@ | |||
codecov | |||
flake8 | |||
flake8==4.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question]
What is the usage of the requirements files when we have environment ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows the user to choose whether to install with pip or conda.
Too many changes since this PR. Should be noted when a configuration settings need to be updated. |
Description
This configures the pre-commit functionnality: when you prepare a commit, it will automatically run default pre-commit checks and run black and flake8 checks.
Fixes #(issue)
Type of change
Checklist
make lint
make type-check
make tests
make coverage
make doc