Use Results class for PCA Analysis by PicoCentauri · Pull Request #3285 · MDAnalysis/mdanalysis

PicoCentauri · 2021-05-07T10:15:36Z

Fixes #3275

Changes made in this Pull Request:
Move the results of the PCA analysis to the new results class

PR Checklist

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

pep8speaks · 2021-05-07T10:15:39Z

Hello @PicoCentauri! 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-05-27 19:54:14 UTC

codecov · 2021-05-07T10:35:04Z

Codecov Report

Merging #3285 (a2f5287) into develop (560feed) will increase coverage by 0.73%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #3285      +/-   ##
===========================================
+ Coverage    92.88%   93.61%   +0.73%     
===========================================
  Files          171      176       +5     
  Lines        22323    22835     +512     
  Branches      3224     3224              
===========================================
+ Hits         20734    21378     +644     
- Misses        1043     1406     +363     
+ Partials       546       51     -495

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/pca.py	`100.00% <100.00%> (+0.66%)`	⬆️
package/MDAnalysis/lib/NeighborSearch.py	`96.42% <0.00%> (-3.58%)`	⬇️
package/MDAnalysis/topology/tpr/obj.py	`96.96% <0.00%> (-3.04%)`	⬇️
...onality_reduction/DimensionalityReductionMethod.py	`97.05% <0.00%> (-2.95%)`	⬇️
package/MDAnalysis/analysis/density.py	`82.45% <0.00%> (-0.08%)`	⬇️
package/MDAnalysis/transformations/base.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/tests/__init__.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/analysis/legacy/x3dna.py	`0.00% <0.00%> (ø)`
package/MDAnalysis/tests/datafiles.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/due.py	`75.00% <0.00%> (ø)`
... and 90 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 560feed...a2f5287. Read the comment docs.

IAlibay

Couple comments, not too sure about the docstring change (it looks good to me, but I don't remember if that was the direction we wanted to go to or not).

package/MDAnalysis/analysis/pca.py

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

orbeckst

lgtm

package/MDAnalysis/analysis/pca.py

IAlibay

Keeping my request changes status as a blocking comment because I think the mean_atoms path needs further thought (at least on how we will handle it for 2.0).

It's not really a problem with this PR, but that code path really doesn't seem to work (at least me atm), and I'd prefer we fix #2728 before we do any further changes. We could merge as-is and then promise to fix #2728 before the full release too, so I'll just leave it up for discussion and go with the majority opinion.

package/MDAnalysis/analysis/pca.py

orbeckst · 2021-05-08T22:12:18Z

Not sure where we are with the soft code freeze. However, if we want PCA for the workshop then this PR needs to go in, even if mean atoms still needs fixing.

However, we can always add fixes, we have a hard time putting in breaking changes so I would suggest to move forward with this PR.

I’m sorry, I have limited bandwidth today to do more.

IAlibay · 2021-05-08T22:18:34Z

Not sure where we are with the soft code freeze. However, if we want PCA for the workshop then this PR needs to go in, even if mean atoms still needs fixing.

However, we can always add fixes, we have a hard time putting in breaking changes so I would suggest to move forward with this PR.

I’m sorry, I have limited bandwidth today to do more.

Sorry, I can't do any more today. If @fiona-naughton or one of the @MDAnalysis/coredevs wants to push this through then that'd be great. See discord for more on what our priority merges should be.

fiona-naughton · 2021-05-09T05:41:39Z

If it's agreed that mean_atoms should have the average positions as calculated/provided to self.means, then that could be fixed by making a dummy atom group and populating the positions with the values from means something like I'm currently doing in #3296 (thought there I didn't need to worry about atom typing etc).

But the updated docs here reflects that it's just the atom selection rather than implying anything about positions and it's not used anywhere else, so the only issue I can see is the name being misleading - so ultimately I'd say it's fine to just leave it as is. Eventually I think we can get rid of it, or just return the position array instead, but that can wait for another day.

tl;dr I say leave mean_atoms for now and decide what to do with it later; everything else lgtm

Co-authored-by: Fiona Naughton <fiona.naughton@asu.edu>

orbeckst · 2021-05-26T21:55:13Z

Will look at this PR as soon as PR #3296 is merged.

orbeckst · 2021-05-26T21:56:12Z

Note that PR #3296 removes mean_atoms.

orbeckst · 2021-05-26T22:49:06Z

@fiona-naughton @IAlibay please re-review after removal of mean_atoms. Thanks.

package/MDAnalysis/analysis/pca.py

orbeckst · 2021-05-26T23:12:30Z

The docs for MDAnalysis.analysis.pca.PCA.cumulative_overlap and rmsip contain a badly rendered See Also section. Would you be able to fix these as well? I am appealing to your 🇩🇪 sense for tidiness ;-).

orbeckst

incorrect links in See Also ... maybe my suggestions fix it

package/MDAnalysis/analysis/pca.py

orbeckst · 2021-05-27T16:20:16Z

@fiona-naughton @IAlibay I assume you're knackered after non-stop workshopping but if you could have a look if this PR finds your approval then we could make another big check mark. (Fixing docs is ongoing)

PicoCentauri · 2021-05-27T16:28:24Z

Thanks, for fixing the link @orbeckst!

package/MDAnalysis/analysis/pca.py

IAlibay

Just the one change, otherwise lgtm (I'll approve so I'm not blocking).

@IAlibay

thanks @IAlibay

orbeckst · 2021-05-27T18:45:28Z

I am messing up with the docs — I'll now fix this for once and for all.

fiona-naughton

lgtm :)

orbeckst · 2021-05-27T20:52:29Z

@IAlibay , ready for you; the docs for rmsip and cumulative_overlap finally show up, too!

IAlibay · 2021-05-27T21:00:17Z

Thanks for all the work here @PicoCentauri and @orbeckst !

Use results for pca

3232393

PicoCentauri changed the title ~~Use results for pca~~ Use Results class for PCA Analysis May 7, 2021

Added tests

2afaa09

IAlibay requested changes May 7, 2021

View reviewed changes

PicoCentauri added 4 commits May 7, 2021 12:48

Updated CHANGELOG

ce4a17e

Docs updated

c2f16d2

PEP8

813af65

Fixed forgotten rename

c915fa6

orbeckst assigned IAlibay May 7, 2021

PicoCentauri and others added 3 commits May 8, 2021 00:23

check object instance

2f4c858

Update package/MDAnalysis/analysis/pca.py

45c238e

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

PEP8

9194116

orbeckst approved these changes May 7, 2021

View reviewed changes

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

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

Merge branch 'develop' into 3275-results-pca

a243c7c

orbeckst added API Component-Analysis labels May 7, 2021

Merge branch 'develop' into 3275-results-pca

91d42d6

IAlibay reviewed May 8, 2021

View reviewed changes

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

fiona-naughton requested changes May 8, 2021

View reviewed changes

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

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

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

Apply suggestions from fiona

8320edd

Co-authored-by: Fiona Naughton <fiona.naughton@asu.edu>

fiona-naughton mentioned this pull request May 11, 2021

Allow PCA to run with given reference mean values #3296

Merged

4 tasks

orbeckst added 2 commits May 26, 2021 15:46

Merge branch 'develop' into 3275-results-pca

d09a2b9

removed deprecation test for mean_atoms

d843801

orbeckst reviewed May 26, 2021

View reviewed changes

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

Smooth and tidy docs

3dfdb15

orbeckst requested changes May 27, 2021

View reviewed changes

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

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

Apply suggestions from code review

72a2bb0

IAlibay reviewed May 27, 2021

View reviewed changes

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

IAlibay approved these changes May 27, 2021

View reviewed changes

orbeckst added 2 commits May 27, 2021 09:52

doc fix

6fadd78

thanks @IAlibay

pep8 trailing whitespace fixes

9b7f6a6

orbeckst approved these changes May 27, 2021

View reviewed changes

cross-referenced rmsip <-> cumulative_overlap

ef1c00d

fixed docs, for good

a2f5287

fiona-naughton approved these changes May 27, 2021

View reviewed changes

IAlibay merged commit 13e1a0f into MDAnalysis:develop May 27, 2021

IAlibay added the enhancement label Sep 25, 2023

PicoCentauri deleted the 3275-results-pca branch June 5, 2024 15:55

Conversation

PicoCentauri commented May 7, 2021 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

pep8speaks commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-05-27 19:54:14 UTC

Uh oh!

codecov bot commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst commented May 8, 2021

Uh oh!

IAlibay commented May 8, 2021

Uh oh!

fiona-naughton commented May 9, 2021

Uh oh!

orbeckst commented May 26, 2021

Uh oh!

orbeckst commented May 26, 2021

Uh oh!

orbeckst commented May 26, 2021

Uh oh!

Uh oh!

orbeckst commented May 26, 2021

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orbeckst commented May 27, 2021

Uh oh!

PicoCentauri commented May 27, 2021

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented May 27, 2021

Uh oh!

fiona-naughton left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented May 27, 2021

Uh oh!

IAlibay commented May 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

PicoCentauri commented May 7, 2021 •

edited by orbeckst

Loading

pep8speaks commented May 7, 2021 •

edited

Loading

codecov bot commented May 7, 2021 •

edited

Loading