Add cylindrical selection test using PeriodicKDTree #5194
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.
|
Hi @rishinamdev17 - we can't review this PR at all given the tests are currently failing. Please look into ensuring the tests are passing (ideally you wouldn't open a new PR for a new test that doesn't pass to begin with). |
|
hi @BradyAJohnston The cylayer selection appears to internally route through PointSelection and expects additional tokens (cutoff) that aren’t obvious from the public API, which results in a float(None) error even with valid-looking syntax like cylayer … index 0. I don’t want to force a workaround without understanding the intended grammar, so I’m pausing further changes until I get clarification on the correct cylayer selection form to use for this test. Happy to update once advised. |
|
You are not currently using
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5194 +/- ##
===========================================
- Coverage 92.72% 92.72% -0.01%
===========================================
Files 180 180
Lines 22472 22474 +2
Branches 3188 3189 +1
===========================================
+ Hits 20838 20839 +1
- Misses 1176 1177 +1
Partials 458 458 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for your patience @BradyAJohnston !! |
| assert_equal( | ||
| u.atoms[:3].chiralities, np.array(["", chirality, ""], dtype="U1") | ||
| ) | ||
| assert_equal(u.atoms[:3].chiralities, np.array(["", chirality, ""], dtype="U1")) |
There was a problem hiding this comment.
Are all these unrelated whitespace changes really needed?
There was a problem hiding this comment.
Those whitespace changes are unrelated to my test. I'll revert them and keep the PR focused only on the cylayer test.
| from MDAnalysis import SelectionError, SelectionWarning | ||
| from MDAnalysis.core.selection import Parser | ||
| from MDAnalysis.lib.distances import distance_array | ||
| from MDAnalysis.lib.pkdtree import PeriodicKDTree |
There was a problem hiding this comment.
Yes you are right!! that import is no lomger needed after simplifying the test and I have removed it..
| def universe(): | ||
| u = mda.Universe(PSF, DCD) | ||
| u.dimensions = np.array([100.0, 100.0, 100.0, 90.0, 90.0, 90.0]) | ||
| return u |
There was a problem hiding this comment.
Creating a fixture for use in a single test case seems a bit much, especially when you're repeating the setting of dimensions below anyway.
There was a problem hiding this comment.
I have combined the universe setup into the test and removed the duplicate dimensions setting.
|
There are still a lot of formatting changes that should be undone. The test you have also now created no longer really tests what you set out to do - it is just testing the parsing of a selection and not that it matches against using the PeriodicKDTree. |
|
@BradyAJohnston thanks for moving the PR along. I assigned it to you, given that you're already doing the work of the PR shepherd. Thank you! |
|
Hi @rishinamdev17 , I noticed you are struggling with black , you might want to look into CONTRIBUTING.md. |
5 participants
Fixes #1736
Changes made in this Pull Request:
cylayer) usingPeriodicKDTreePR Checklist
package/CHANGELOGfile updated? (not required for test-only change)package/AUTHORS? (optional for small contributions)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.