Fix bug in PCA trajectory iteration -- avoid explicit usage of self.start #4423

marinegor · 2024-01-12T14:23:54Z

This fixes a bug which made impossible the
Fixes #4425

Changes made in this Pull Request:

access first frame in trajectory with self._sliced_trajectory[0] instead of self._trajectory[self.start], since self.start is None if run is initiated with a.run(frames=[1,2]).

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--4423.org.readthedocs.build/en/4423/

…tart

pep8speaks · 2024-01-12T14:24:00Z

Hello @marinegor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/analysis/pca.py:

Line 145:78: W291 trailing whitespace

Comment last updated at 2024-01-18 16:56:20 UTC

github-actions · 2024-01-12T14:26:12Z

Linter Bot Results:

Hi @marinegor! 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/7573356413/job/20625313741

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

fix formatting changes

codecov · 2024-01-12T14:40:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (24abb16) 93.36% compared to head (7e915a4) 93.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##           develop    #4423     +/-   ##
==========================================
  Coverage    93.36%   93.36%             
==========================================
  Files          171      185     +14     
  Lines        21736    22850   +1114     
  Branches      4012     4012             
==========================================
+ Hits         20293    21334   +1041     
- Misses         954     1027     +73     
  Partials       489      489

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

package/MDAnalysis/analysis/pca.py

hmacdope · 2024-01-15T00:10:55Z

Could you also raise an issue @marinegor just so we have a record of what went wrong?

marinegor · 2024-01-15T11:12:40Z

hi @hmacdope , I raised the issue -- should I do anything else?
I'm not sure if the discussion above is regarding this PR, especially since I feel I can't contribute much to it😁

marinegor · 2024-01-17T20:54:08Z

So anyway, I added versionchanged to the class documentation describing what's going on -- hopefully that'll be enough for the PR!

hmacdope

@marinegor could you add

When using mean selections, the first frame of the selected trajectory slice is used as a reference.

To the main docstring of PCA class? other than that looks good.

hmacdope · 2024-01-17T22:16:25Z

package/MDAnalysis/analysis/pca.py

+       described by #4425 and fixed by #4423). Previously, behaviour was to
+       manually iterate through ``self._trajectory``, which starting #3415 that
+       introduced ``frames`` is wrong, and should use ``self._sliced_trajectory``.
+       It is now fixed.


Suggested change

It is now fixed.

marinegor · 2024-01-17T22:25:21Z

@hmacdope done both (applied suggestion in my commit)

hmacdope

I will approve but would love a review from @IAlibay before going ahead and merging. Well done @marinegor!

IAlibay · 2024-01-17T22:31:07Z

@hmacdope happy to have a quick look tomorrow, could you do a review request for me please? (sorry I'm on phone)

hmacdope · 2024-01-17T22:42:57Z

@hmacdope happy to have a quick look tomorrow, could you do a review request for me please? (sorry I'm on phone)

I think you are already requested.

IAlibay

Couple of small change suggestions, otherwise lgtm!

package/CHANGELOG

IAlibay · 2024-01-18T11:00:46Z

package/MDAnalysis/analysis/pca.py

@@ -142,7 +142,8 @@ class PCA(AnalysisBase):
    generates the principal components of the backbone of the atomgroup and
    then transforms those atomgroup coordinates by the direction of those
    variances. Please refer to the :ref:`PCA-tutorial` for more detailed
-    instructions.
+    instructions. When using mean selections, the first frame of the selected 


this is great thanks!

package/MDAnalysis/analysis/pca.py

testsuite/MDAnalysisTests/analysis/test_pca.py

Co-authored-by: Irfan Alibay <[email protected]>

marinegor · 2024-01-18T16:34:14Z

@IAlibay

Since it's a bit of a smoke test of "will this work", and we don't assert on anything, could you add mean=True explicitly here?

Actually, mean=None (=the default option) ensures I run this part of the code afaik (I checked it with `assert False on L286). So I reverted back to my version of the test.

IAlibay · 2024-01-18T16:43:54Z

@IAlibay

Since it's a bit of a smoke test of "will this work", and we don't assert on anything, could you add mean=True explicitly here?

Actually, mean=None (=the default option) ensures I run this part of the code afaik (I checked it with `assert False on L286). So I reverted back to my version of the test.

Sorry maybe I didn't explain my point well enough - None currently has that behaviour, but given we want to ensure it's always going to be called forever into the future, then setting True may be better.

That being said - I'm not particularly too fussed here, it's just nice to enforce a branch decision rather than rely on defaults.

marinegor · 2024-01-18T17:33:23Z

@IAlibay I see what you mean now -- added explicit frames=None here.
Btw, I firmly believe all the tests are ok but something wrong with the CI💔

RMeli · 2024-01-20T21:38:34Z

/azp run

azure-pipelines · 2024-01-20T21:38:44Z

Azure Pipelines successfully started running 1 pipeline(s).

RMeli · 2024-01-20T22:04:02Z

Btw, I firmly believe all the tests are ok but something wrong with the CI💔

Looked like a timeout. Re-running Azure Pipelines completed successfully.

marinegor added 2 commits January 12, 2024 15:17

Fix bug in PCA trajectory iteration -- avoid explicit usage of self.s…

211cbcf

…tart

update changelog and tests for PCA fix

d4de910

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

Update test_pca.py

be05e9b

fix formatting changes

hmacdope reviewed Jan 14, 2024

View reviewed changes

package/MDAnalysis/analysis/pca.py Show resolved Hide resolved

Replace self._u.trajectory[...] with self._sliced_trajectory

9743827

marinegor requested review from IAlibay and hmacdope January 17, 2024 20:54

hmacdope requested changes Jan 17, 2024

View reviewed changes

Update documentaiton

e146863

hmacdope approved these changes Jan 17, 2024

View reviewed changes

IAlibay approved these changes Jan 18, 2024

View reviewed changes

IAlibay and others added 4 commits January 18, 2024 11:14

Merge branch 'develop' into bugfix/pca-frames-iteration

dbe2dbc

Update testsuite/MDAnalysisTests/analysis/test_pca.py

a4b2f4a

Co-authored-by: Irfan Alibay <[email protected]>

Update package/MDAnalysis/analysis/pca.py

0c95e34

Co-authored-by: Irfan Alibay <[email protected]>

Update package/CHANGELOG

39965ee

Co-authored-by: Irfan Alibay <[email protected]>

revert back to testing with mean=None

3b270c8

Enforce mean=None when testing with frames

7e915a4

RMeli merged commit 8ec8c32 into MDAnalysis:develop Jan 20, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in PCA trajectory iteration -- avoid explicit usage of self.start #4423

Fix bug in PCA trajectory iteration -- avoid explicit usage of self.start #4423

marinegor commented Jan 12, 2024 •

edited

Loading

pep8speaks commented Jan 12, 2024 •

edited

Loading

github-actions bot commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

hmacdope commented Jan 15, 2024

marinegor commented Jan 15, 2024

marinegor commented Jan 17, 2024

hmacdope left a comment

hmacdope Jan 17, 2024

marinegor commented Jan 17, 2024

hmacdope left a comment

IAlibay commented Jan 17, 2024

hmacdope commented Jan 17, 2024

IAlibay left a comment

IAlibay Jan 18, 2024

marinegor commented Jan 18, 2024

IAlibay commented Jan 18, 2024

marinegor commented Jan 18, 2024

RMeli commented Jan 20, 2024

azure-pipelines bot commented Jan 20, 2024

RMeli commented Jan 20, 2024

Fix bug in PCA trajectory iteration -- avoid explicit usage of self.start #4423

Fix bug in PCA trajectory iteration -- avoid explicit usage of self.start #4423

Conversation

marinegor commented Jan 12, 2024 • edited Loading

PR Checklist

Developers certificate of origin

pep8speaks commented Jan 12, 2024 • edited Loading

Comment last updated at 2024-01-18 16:56:20 UTC

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

Linter Bot Results:

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

hmacdope commented Jan 15, 2024

marinegor commented Jan 15, 2024

marinegor commented Jan 17, 2024

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope Jan 17, 2024

Choose a reason for hiding this comment

marinegor commented Jan 17, 2024

hmacdope left a comment

Choose a reason for hiding this comment

IAlibay commented Jan 17, 2024

hmacdope commented Jan 17, 2024

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Jan 18, 2024

Choose a reason for hiding this comment

marinegor commented Jan 18, 2024

IAlibay commented Jan 18, 2024

marinegor commented Jan 18, 2024

RMeli commented Jan 20, 2024

azure-pipelines bot commented Jan 20, 2024

RMeli commented Jan 20, 2024

marinegor commented Jan 12, 2024 •

edited

Loading

pep8speaks commented Jan 12, 2024 •

edited

Loading

github-actions bot commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading