-
Notifications
You must be signed in to change notification settings - Fork 744
Modernize assertions in test_distances.py (part of #3743) #5158
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 assertions in test_distances.py (part of #3743) #5158
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5158 +/- ##
===========================================
- Coverage 92.72% 92.71% -0.02%
===========================================
Files 180 180
Lines 22458 22457 -1
Branches 3186 3186
===========================================
- Hits 20824 20820 -4
- Misses 1177 1179 +2
- Partials 457 458 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR is ready for review. All tests pass locally, and the only CI failure was a cancelled job due to duplicate workflow runs by GitHub. @orbeckst @RMeli @marinegor — Please let me know if anything else is needed. Thanks! |
marinegor
left a comment
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.
@Pranay096 thanks for the PR, this one is also accepted.
If you're planning to work on them file by file, I'd be in favor of choosing some strategy to chunk them together, since there are 70 files that need modernization, and 860 occurrences total. I'd think about splitting them logically (i.e. by module or finer) into ~10-ish PRs, because 70 PRs would be quite overwhelming in terms of review time (but perhaps it's just me -- @MDAnalysis/coredevs ?)
|
Hi @marinegor @RMeli @orbeckst Plan I’m thinking of submitting 10–15 small PRs, each focused on one module area to keep reviews manageable and avoid large changes. Quick Question Some deprecated patterns appear in the package code too. I’ll follow whichever direction you prefer. Thanks! |
How did you actually do that? I found only 70 files:
Which exactly?
The issue you're working in only regarding testing, so within this issue I'd limit your work. If you decide to also replace it in other places, please create a separate issue for that. |
|
@Pranay096 may I suggest possibly thinking about targetting another issue. Issue #3743 should be considered as a large "starter" isssue that several folks over time would contribute to, not something a single individual should be doing in one go (at least not until we actually hit a deprecation deadline). I would urge you to consider looking at some of the more complicated issues on our tracker. |
|
Thank you @marinegor and @IAlibay for the explanation and advice; it is greatly appreciated. As for the number of files, my initial scan contained a few other deprecated patterns scattered around the repository so the number was bigger. Your rg command clarified that only ~70 files are relevant given the specific assert_almost_equal update and I'll strictly keep my contributions for this issue limited to the test suite. If I catch similar patterns in the package code, I'll open a separate issue without intermixing the changes. I also appreciate the insight into Issue #3743, and I now realize that it is supposed to be a long-term issue of collaboration, not something for one contributor to handle; I will definitely take your suggestion and start investigating some of the harder issues on the tracker where I can make more substantial contributions. I truly value both of your insights, as it helps me stay aligned with the expectations of the project and make my contributions as helpful as possible. |
Contributes to #3743
Changes made in this Pull Request:
testsuite/MDAnalysisTests/analysis/test_distances.pyassert_almost_equal/assert_array_almost_equalwithnumpy.testing.assert_allcloseandassert_array_equalwhere appropriatePR Checklist
package/CHANGELOGfile updated?package/AUTHORS?Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5158.org.readthedocs.build/en/5158/