Issue #3467: Replaced .coordinates() with .positions #3479

pranjii · 2021-12-08T23:34:01Z

Changes made in this Pull Request:
-Replaced .coordinates() with .positions

PR Checklist

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

codecov · 2021-12-08T23:55:57Z

Codecov Report

Merging #3479 (5d1f343) into develop (5bf6359) will increase coverage by 3.59%.
The diff coverage is 0.00%.

❗ Current head 5d1f343 differs from pull request most recent head 63b60f6. Consider uploading reports for the commit 63b60f6 to get more accurate results

@@             Coverage Diff             @@
##           develop    MDAnalysis/mdanalysis#3479      +/-   ##
===========================================
+ Coverage    90.15%   93.75%   +3.59%     
===========================================
  Files          176      176              
  Lines        23169    23169              
  Branches      3291     3297       +6     
===========================================
+ Hits         20889    21723     +834     
+ Misses        2280     1395     -885     
- Partials         0       51      +51

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/encore/covariance.py	`87.69% <0.00%> (ø)`
package/MDAnalysis/coordinates/base.py	`95.47% <0.00%> (+0.11%)`	⬆️
package/MDAnalysis/coordinates/TRJ.py	`97.88% <0.00%> (+0.46%)`	⬆️
package/MDAnalysis/lib/util.py	`90.55% <0.00%> (+0.56%)`	⬆️
package/MDAnalysis/core/universe.py	`97.95% <0.00%> (+3.62%)`	⬆️
package/MDAnalysis/analysis/align.py	`98.32% <0.00%> (+5.36%)`	⬆️
package/MDAnalysis/core/selection.py	`98.78% <0.00%> (+5.55%)`	⬆️
package/MDAnalysis/topology/guessers.py	`99.22% <0.00%> (+6.97%)`	⬆️
package/MDAnalysis/analysis/psa.py	`89.27% <0.00%> (+10.13%)`	⬆️
package/MDAnalysis/analysis/hole2/utils.py	`76.10% <0.00%> (+50.31%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bf6359...63b60f6. Read the comment docs.

pranjii · 2021-12-09T19:42:44Z

@orbeckst Hey one of my tests seem to be failing could you help me out fixing it?

IAlibay

Thanks for opening this PR @pranjii

The failing test is essentially an indication that there is a section of the code not covered by tests. Ideally we would want to add a test here to make sure that the code is working as expected.

Please also do add yourself to AUTHORS and update the CHANGELOG accordingly.

IAlibay · 2021-12-10T09:18:23Z

package/MDAnalysis/analysis/encore/covariance.py

@@ -215,7 +215,7 @@ def covariance_matrix(ensemble,
        # Select the same atoms in reference structure
        reference_atom_selection = reference.select_atoms(
            ensemble.get_atom_selection_string())
-        reference_coordinates = reference_atom_selection.atoms.coordinates()
+        reference_coordinates = reference_atom_selection.atoms.positions


The failed codecov check is indicating that the this line isn't covered by a test. This explains why tests didn't identify that atoms.coordinates() no longer existed.

If possible could you add a test that checks this code path?

pranjii · 2021-12-10T15:19:15Z

Thanks for opening this PR @pranjii

The failing test is essentially an indication that there is a section of the code not covered by tests. Ideally we would want to add a test here to make sure that the code is working as expected.

Please also do add yourself to AUTHORS and update the CHANGELOG accordingly.

working on it

pep8speaks · 2021-12-10T21:27:06Z

Hello @pranjii! 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 2021-12-10 21:40:57 UTC

pranjii · 2021-12-10T22:32:34Z

@IAlibay Seems like I am not the guy who can do the tests. Should I revert my commits or will the PR be closed?

IAlibay · 2021-12-10T22:36:06Z

@IAlibay Seems like I am not the guy who can do the tests. Should I revert my commits or will the PR be closed?

Can you go into more details? If you want to walk us through what you think is going wrong I'm sure we can do our best to guide you towards tests that pass.

So this is your current error E AttributeError: module 'MDAnalysis.analysis.encore' has no attribute 'covariance_matrix'. Can you try to write a comment here to explain what you're trying to do and then we can start from there?

orbeckst · 2021-12-10T22:42:46Z

Seems like I am not the guy who can do the tests.

I approved running the tests — if this is what you mean?

We would want you to write actual tests for MDAnalysis that are run to check your fix. Have you written tests with pytests before? See https://userguide.mdanalysis.org/stable/testing.html to learn more about tests and to get started.

pranjii · 2021-12-11T01:21:21Z

@IAlibay Thank u. I will probably need some time to figure it all out and thank u for offering ur help. Right now I have a question:

I didn't see any of the functions in covariance.py tested. Is it supposed to be so? If yes is the location right now ok or should is there some other location you would prefer?

Right now the problem is that I was unaware of how advanced was the mathematical concept I was dealing with, the E AttributeError: module 'MDAnalysis.analysis.encore' has no attribute 'covariance_matrix'. was a totally silly error that I made during the pep8 changes and due to my lack of experience in testing functions but I will work on each of the issue and will deliver the final product.

Thank u for the support.

orbeckst · 2022-01-05T00:25:34Z

Hello @pranjii , are you still working on this PR?

IAlibay · 2022-01-05T00:28:15Z

I didn't see any of the functions in covariance.py tested. Is it supposed to be so? If yes is the location right now ok or should is there some other location you would prefer?

Sorry I failed to respond to this. The tests should be under testsuite/MDAnalysisTests/analysis/test_encore.py.

ojeda-e · 2022-01-13T08:10:16Z

@orbeckst @IAlibay I noticed that lines 216-217 in covariance.py have no previous coverage, then a brand new unit test should be added. However, these lines are a bit problematic because of two main reasons:

get_atom_selection_string() is not an attribute of Universe.
It seems like the if statement doesn't do what the docstrings indicate. With no reference, it should run a covariance calculation using distance to the mean. I may be wrong, but shouldn't it be something like this?

    if reference is not None:
        reference_coordinates = reference.atoms.positions.flatten()
    else:
        # Covariance calculation using distance to the mean
        reference_coordinates = coordinates - np.mean(coordinates, axis=0)

Additionally, tests for similarity uses covariance in test_encore.py, and if I am correct about item (2), fixing it would mean that additional unit tests should be modified because covariance_matrix, ml_covariance_estimator, and shrinkage_covariance_estimator are used to calculate harmonic ensemble similarity, hence there is a possibility that HES tests will fail.
If I am correct I could help with this issue. Otherwise, I'd appreciate if you let me know what I am missing here, thanks.

orbeckst · 2022-01-13T23:38:37Z

Is ensemble really a Universe or something else (even though the docs say Universe object)?

…

Am 1/13/22 um 01:10 schrieb Estefania Barreto-Ojeda ***@***.***>: get_atom_selection_string() is not an attribute of Universe.

ojeda-e · 2022-01-14T01:22:35Z

Is ensemble really a Universe or something else (even though the docs say Universe object)?

Yes, an ensemble is a Universe defined with timeseries as a bound method. For example:

from MDAnalysisTests.datafiles import DCD, PSF
import MDAnalysis as mda

traj = mda.Universe(PSF, DCD)
ens1 = mda.Universe(PSF, traj.trajectory.timeseries())

I don't know if the get_atom_selection_string() was an attribute of Universe in previous versions of MDAnalysis, but I can't find it anywhere else in the library.
Since an ensemble is a Universe, I guess lines 216-217 are meant to be:

reference_atom_selection = ensemble.select_atoms(reference)

instead of

reference_atom_selection = reference.select_atoms(
            ensemble.get_atom_selection_string())

If I am understanding correctly. (Spoiler alert: I may not)

jbarnoud · 2022-02-03T17:55:41Z

From what I can tell, the call to get_atom_selection_string was introduced in 5d914cc and at no point was the method defined. It looks to me that this part of the code never worked and is probably a leftover from when ENCORE was developed outside of MDAnalysis. Maybe @mtiberti has an idea about it?

jbarnoud · 2022-02-17T13:17:55Z

Fixing ENCORE is out of scope for this PR. I opened MDAnalysis/mdaencore#29 and #3539 to track that issue.

Regarding the original issue, the code about rms is preceded by a comment that says:

# ag.positions is the new syntax
# but older commit hashes will need to use
# ag.coordinates()

We run daily benchmarks, the result of which are visible here: https://www.mdanalysis.org/benchmarks/. The benchmarks need to be ale to run on old commits as well, some of which being older than the positions attribute. Hence the try/except. We recently changed the minimum commit we benchmark to a version that do have the attribute (MDAnalysis/benchmarks#12 (comment)) so the try/except is not useful anymore and became dead code. In this instance, replacing .coordinates() by .positions means repeating the same code twice. Instead, we should remove the dead code.

The next step would be to:

remove the dead code in the rms benchmark
remove the broken test, it will be added back when we know how to fix the broken code
Update the CHANGELOG
Add yourself to the AUTHOR file

jbarnoud · 2022-02-17T13:24:49Z

By the way, MDAnalysis is currently in the process of evaluating a potential change in its license. As part of this we are asking that all new-ish contributors introduce themselves (with an email address that they regularly monitor) to the MDAnalysis developer list before we accept their contributions. Please include your github handle and a link to this pull request. The aim is that we then have a means of contacting the developers should we decide to change our license in the near future.

Thank you.

orbeckst · 2022-03-29T01:25:10Z

Hello @pranjii are you still working on this PR?

There hasn't been any activity (including questions) for over a month so unless we hear anything over the next 4 days we will close this PR as stale.

orbeckst · 2022-04-01T17:22:26Z

I am closing this PR due to inactivity.

PR #3576 is now considered primary PR for issue #3467.

Issue MDAnalysis#3467

5d1f343

github-actions bot added Component-Analysis performance labels Dec 8, 2021

IAlibay requested changes Dec 10, 2021

View reviewed changes

MDAnalysis#3467

4436a07

pranjii added 2 commits December 11, 2021 03:05

pep8 edits

f08867d

pep8 edits 2

63b60f6

This was referenced Sep 6, 2023

Test coverage for ENCORE needs to be increased MDAnalysis/mdaencore#29

Open

ENCORE uses an non existing method #3539

Closed

orbeckst mentioned this pull request Mar 29, 2022

updated atoms.coordinates() with atoms.positions #3467 #3576

Closed

4 tasks

orbeckst added close? Evaluate if issue/PR is stale and can be closed. and removed performance labels Mar 31, 2022

orbeckst closed this Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3467: Replaced .coordinates() with .positions #3479

Issue #3467: Replaced .coordinates() with .positions #3479

pranjii commented Dec 8, 2021 •

edited

Loading

codecov bot commented Dec 8, 2021 •

edited

Loading

pranjii commented Dec 9, 2021

IAlibay left a comment

IAlibay Dec 10, 2021

pranjii commented Dec 10, 2021

pep8speaks commented Dec 10, 2021 •

edited

Loading

pranjii commented Dec 10, 2021

IAlibay commented Dec 10, 2021

orbeckst commented Dec 10, 2021

pranjii commented Dec 11, 2021

orbeckst commented Jan 5, 2022

IAlibay commented Jan 5, 2022

ojeda-e commented Jan 13, 2022

orbeckst commented Jan 13, 2022 via email

ojeda-e commented Jan 14, 2022

jbarnoud commented Feb 3, 2022

jbarnoud commented Feb 17, 2022

jbarnoud commented Feb 17, 2022

orbeckst commented Mar 29, 2022

orbeckst commented Apr 1, 2022

Issue #3467: Replaced .coordinates() with .positions #3479

Issue #3467: Replaced .coordinates() with .positions #3479

Conversation

pranjii commented Dec 8, 2021 • edited Loading

PR Checklist

codecov bot commented Dec 8, 2021 • edited Loading

Codecov Report

pranjii commented Dec 9, 2021

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Dec 10, 2021

Choose a reason for hiding this comment

pranjii commented Dec 10, 2021

pep8speaks commented Dec 10, 2021 • edited Loading

Comment last updated at 2021-12-10 21:40:57 UTC

pranjii commented Dec 10, 2021

IAlibay commented Dec 10, 2021

orbeckst commented Dec 10, 2021

pranjii commented Dec 11, 2021

orbeckst commented Jan 5, 2022

IAlibay commented Jan 5, 2022

ojeda-e commented Jan 13, 2022

orbeckst commented Jan 13, 2022 via email

ojeda-e commented Jan 14, 2022

jbarnoud commented Feb 3, 2022

jbarnoud commented Feb 17, 2022

jbarnoud commented Feb 17, 2022

orbeckst commented Mar 29, 2022

orbeckst commented Apr 1, 2022

pranjii commented Dec 8, 2021 •

edited

Loading

codecov bot commented Dec 8, 2021 •

edited

Loading

pep8speaks commented Dec 10, 2021 •

edited

Loading