Feat: Add `n_initial_contacts` attribute and document `Contacts` class attributes #4415

HeetVekariya · 2024-01-07T06:34:07Z

Fixes #2604

Changes made in this Pull Request:

Added n_initial_contacts attribute to Contacts class.
Documented Contacts class attributes to its doc string.

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

📚 Documentation preview 📚: https://mdanalysis--4415.org.readthedocs.build/en/4415/

pep8speaks · 2024-01-07T06:34:13Z

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

github-actions · 2024-01-07T06:36:22Z

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.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/7677106155/job/20925443250

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

codecov · 2024-01-07T06:45:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e189a90) 93.37% compared to head (a91893d) 93.38%.

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.
📢 Have feedback on the report? Share it here.

HeetVekariya · 2024-01-07T06:58:55Z

I haven't added the testing code, which is needed to be added in the test_contacts.py (here).

Questions:

Do i need to just compare single values for multiple examples using assert ?
How can i test if my "Testing code" is working, in the local ?
Can any of the maintainer provide me the possible cases which should be tested with expected values as i don't about it ?
like below,

@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)

Is there any change required in the doc string for version changed ?

HeetVekariya · 2024-01-19T09:18:13Z

@lilyminium can you give me examples which i have to check in the test code, like i have shown in my above message.
Currently for trial purpose i am using below code which gives 0 as output, which i think may be not good for testing.

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)

lilyminium

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 with assert would do here! I say two because there are two cases you've specified here -- one if len(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 run pytest . 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 to mdanalysis/testsuite/MDAnalysisTests/analysis and run pytest 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.

package/MDAnalysis/analysis/contacts.py

HeetVekariya · 2024-01-23T13:59:43Z

@lilyminium Thank you for reviewing.
I have added the tests, can you please review it whenever you are available.

lilyminium

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 :-)

package/MDAnalysis/analysis/contacts.py

HeetVekariya · 2024-01-24T14:01:50Z

@lilyminium Thank you for guiding me. I have made changes as requested.

RMeli · 2024-01-24T15:38:07Z

Restarted the failing test because of timeout.

lilyminium

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!

lilyminium · 2024-01-27T05:45:48Z

package/MDAnalysis/analysis/contacts.py

+        if len(self.initial_contacts) > 0:
+            self.n_initial_contacts = self.initial_contacts[0].sum()


I think you pointed out that self.initial_contacts must always be longer than 0 -- do you still need the if here?

Fair, i remove the if statement.

lilyminium

Thanks @HeetVekariya! Everything LGTM pending tests working -- thank you very much for contributing this fix!

HeetVekariya · 2024-01-27T07:29:07Z

@lilyminium Thank you for guiding consistentently 🙏
Is there anything left from me on this PR ?

RMeli · 2024-01-29T06:02:44Z

Looks like we had another timeout. I restarted the failed job.

HeetVekariya added 2 commits January 7, 2024 11:51

feat: added n_initial_contacts attribute

db9caef

doc: added description for contacts class attributes

5f04dde

github-actions bot added the Component-Analysis label Jan 7, 2024

remove white spaces

9b414bd

lilyminium requested changes Jan 23, 2024

View reviewed changes

package/MDAnalysis/analysis/contacts.py Outdated Show resolved Hide resolved

HeetVekariya added 2 commits January 23, 2024 18:39

doc: added datatype of r0 attribute

3e41972

test: added test for n_initial_attributes

654ea95

lilyminium requested changes Jan 24, 2024

View reviewed changes

package/MDAnalysis/analysis/contacts.py Outdated Show resolved Hide resolved

HeetVekariya and others added 5 commits January 24, 2024 18:46

Merge branch 'MDAnalysis:develop' into feat/attribute-n-initial-contacts

f32d6f2

refact: removed unused parameter

9d6ab62

refact: removed unwanted else condition

23232db

style: line break at 79 chars

3f7048e

change log

8b3376f

lilyminium requested changes Jan 27, 2024

View reviewed changes

HeetVekariya added 2 commits January 27, 2024 12:37

refact: remove if condition

49d9b08

style: remove white spaces

a91893d

lilyminium approved these changes Jan 27, 2024

View reviewed changes

RMeli merged commit 5a992d7 into MDAnalysis:develop Jan 30, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add `n_initial_contacts` attribute and document `Contacts` class attributes #4415

Feat: Add `n_initial_contacts` attribute and document `Contacts` class attributes #4415

HeetVekariya commented Jan 7, 2024 •

edited

Loading

pep8speaks commented Jan 7, 2024 •

edited

Loading

github-actions bot commented Jan 7, 2024 •

edited

Loading

codecov bot commented Jan 7, 2024 •

edited

Loading

HeetVekariya commented Jan 7, 2024 •

edited

Loading

HeetVekariya commented Jan 19, 2024

lilyminium left a comment

HeetVekariya commented Jan 23, 2024

lilyminium left a comment

HeetVekariya commented Jan 24, 2024

RMeli commented Jan 24, 2024

lilyminium left a comment

lilyminium Jan 27, 2024

HeetVekariya Jan 27, 2024

lilyminium left a comment

HeetVekariya commented Jan 27, 2024

RMeli commented Jan 29, 2024

		if len(self.initial_contacts) > 0:
		self.n_initial_contacts = self.initial_contacts[0].sum()

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

HeetVekariya commented Jan 7, 2024 • edited Loading

PR Checklist

Developers certificate of origin

pep8speaks commented Jan 7, 2024 • edited Loading

Comment last updated at 2024-01-27 07:08:42 UTC

github-actions bot commented Jan 7, 2024 • edited Loading

Linter Bot Results:

codecov bot commented Jan 7, 2024 • edited Loading

Codecov Report

HeetVekariya commented Jan 7, 2024 • edited Loading

Questions:

HeetVekariya commented Jan 19, 2024

lilyminium left a comment

Choose a reason for hiding this comment

HeetVekariya commented Jan 23, 2024

lilyminium left a comment

Choose a reason for hiding this comment

HeetVekariya commented Jan 24, 2024

RMeli commented Jan 24, 2024

lilyminium left a comment

Choose a reason for hiding this comment

lilyminium Jan 27, 2024

Choose a reason for hiding this comment

HeetVekariya Jan 27, 2024

Choose a reason for hiding this comment

lilyminium left a comment

Choose a reason for hiding this comment

HeetVekariya commented Jan 27, 2024

RMeli commented Jan 29, 2024

Feat: Add `n_initial_contacts` attribute and document `Contacts` class attributes #4415

Feat: Add `n_initial_contacts` attribute and document `Contacts` class attributes #4415

HeetVekariya commented Jan 7, 2024 •

edited

Loading

pep8speaks commented Jan 7, 2024 •

edited

Loading

github-actions bot commented Jan 7, 2024 •

edited

Loading

codecov bot commented Jan 7, 2024 •

edited

Loading

HeetVekariya commented Jan 7, 2024 •

edited

Loading