-
Notifications
You must be signed in to change notification settings - Fork 29
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
Adds SLF001 to ruff - private member access #435
Conversation
…on private attr. Updates docstring for added clarity
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #435 +/- ##
===========================================
- Coverage 98.52% 98.51% -0.02%
===========================================
Files 46 46
Lines 2923 2960 +37
===========================================
+ Hits 2880 2916 +36
- Misses 43 44 +1 ☔ View full report in Codecov by Sentry. |
Please can we rename the |
Hi @NicolaCourtier, thanks for the comment. I am planning a quick follow-up PR on this to give those methods the checking they need (although, I think almost all of it is in-place already?). At the moment, they are computed through side-effects, which I will correct, and have them accept the object of signals with checking. I've opened an issue for us to discuss on: #436 Edit: If that's still an issue, I can revert here so that this PR isn't held. |
Will review after #425 - where some of the logic regarding modification of the parameter sets is fixed. |
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.
Thanks for this, @BradyPlanden – this is great and I didn't know such a rule existed for what I felt would be a manual task for a long day! I agree with the code changes largely (without having followed the discussion in #425). I do think CodeCov is flaky, especially when changing a private attribute to a public one – from its annotation in pybop/problems/design_problem.py
, so I would suggest triggering it again just to see whether that resolves or if that's an actual dip in coverage that needs to be fixed.
Co-authored-by: Agriya Khetarpal <[email protected]>
4810699
to
32a3b2b
Compare
Let's agree on a more informative name for |
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.
Thanks for the discussion @BradyPlanden, I've added my comments.
Description
This PR adds the SLF001 rule to ruff linting, ensuring private members are not accessed outside the defining class. At the same time, this PR fixes our current access of private members which break SLF001. The main changes include:
cost._evaluate
andcost._evaluateS1
are renamed tocost.compute
andcost.computeS1
for SLF001 and to align with their functionality.AdamW.lambda
renamed toAdamW.lam
to avoid namespace clash with python lambda.Issue reference
Fixes #434
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:
Type of change
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ nox -s tests
$ nox -s doctest
You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks:
Thank you for contributing to our project! Your efforts help us to deliver great software.