-
Notifications
You must be signed in to change notification settings - Fork 648
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
Feat: Add n_initial_contacts
attribute and document Contacts
class attributes
#4415
Feat: Add n_initial_contacts
attribute and document Contacts
class attributes
#4415
Conversation
Hello @HeetVekariya! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-01-27 07:08:42 UTC |
Linter Bot Results:Hi @HeetVekariya! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4415 +/- ##
==========================================
Coverage 93.37% 93.38%
==========================================
Files 171 185 +14
Lines 21736 22851 +1115
Branches 4012 4012
==========================================
+ Hits 20297 21339 +1042
- Misses 952 1025 +73
Partials 487 487 ☔ View full report in Codecov by Sentry. |
Questions:
@pytest.mark.parametrize('pbc', (True, False))
@pytest.mark.parametrize('name, compound', (('molnums', 'molecules'),
('fragindices', 'fragments')))
# fragment is a fixture defined earlier
def test_center_of_mass_compounds_special(self, fragment,
pbc, name, compound):
ref = [a.center_of_mass() for a in fragment.groupby(name).values()]
com = fragment.center_of_mass(pbc=pbc, compound=compound)
assert_almost_equal(com, ref, decimal=5)
|
import numpy as np
import MDAnalysis as mda
from MDAnalysisTests.datafiles import PSF, DCD # Adjust as per available test files
from MDAnalysis.analysis import contacts # Adjust the import according to your project structure
u = mda.Universe(PSF, DCD) # Load test universe
select = ('protein', 'not protein') # Define atom selections
refgroup = (u.select_atoms('protein'), u.select_atoms('not protein')) # Reference groups
# Instantiate Contacts object
conts = contacts.Contacts(u, select, refgroup)
# Expected number of initial contacts (this should be determined or calculated for your test case)
print(conts.n_initial_contacts) |
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.
Sorry it's taken us some time to get to reviewing this @HeetVekariya, and thank you very much for being patient.
- Yes, I think two fairly simple tests just checking the value of
n_initial_contacts
withassert
would do here! I say two because there are two cases you've specified here -- one iflen(self.initial_contacts) > 0
(lines 465-466), and one if not (lines 467-468). - If you create a development environment, you can additionally install pytest. From there, if you navigate to
mdanalysis/testsuite/MDAnalysisTests/
, you can runpytest .
and that will run all the tests. As the test suite is quite large, this can take quite a while, so it might be faster to navigate tomdanalysis/testsuite/MDAnalysisTests/analysis
and runpytest test_contacts.py
to specifically run the Contacts tests. - You might need to play around with some examples yourself, as I don't know any expected values off the top of your head. The example code you've pasted isn't a very good test case as the PSF/DCD files only contain proteins:
import numpy as np
import MDAnalysis as mda
from MDAnalysisTests.datafiles import PSF, DCD # Adjust as per available test files
from MDAnalysis.analysis import contacts # Adjust the import according to your project structure
u = mda.Universe(PSF, DCD) # Load test universe
select = ('protein', 'not protein') # Define atom selections
refgroup = (u.select_atoms('protein'), u.select_atoms('not protein'))
refgroup[1]
<AtomGroup with 0 atoms>
It would be better to use two groups that contain atoms in both, such as different amino acids (e.g. resname ALA
or resname CYS
). Even better would be to use a system like the TPR/XTC files (from MDAnalysisTests.datafiles import TPR, XTC
and u = mda.Universe(TPR, XTC)
), which contain both protein and solvent.
- I think this change is small enough that it doesn't warrant a
versionchanged
.
|
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 adding the tests @HeetVekariya, they look great. Could you please also test if len(self.initial_contact) == 0
? Then I think this will be good to go!
Also, please add this to CHANGELOG! It's probably an Enhancement :-)
@lilyminium Thank you for guiding me. I have made changes as requested. |
Restarted the failing test because of timeout. |
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 making the change @HeetVekariya, looking pretty good! I just have one comment, and the pep8 bot has noticed some trailing whitespace -- could you just fix that up please? Then I think it's good to go!
if len(self.initial_contacts) > 0: | ||
self.n_initial_contacts = self.initial_contacts[0].sum() |
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 think you pointed out that self.initial_contacts
must always be longer than 0 -- do you still need the if
here?
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.
Fair, i remove the if
statement.
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 @HeetVekariya! Everything LGTM pending tests working -- thank you very much for contributing this fix!
|
Looks like we had another timeout. I restarted the failed job. |
Fixes #2604
Changes made in this Pull Request:
n_initial_contacts
attribute toContacts
class.Contacts
class attributes to its doc string.PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4415.org.readthedocs.build/en/4415/