Skip to content
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.

Sign up for GitHub

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

Jump to bottom

mark analysis.pca.PCA as not parallelizable #4684

Merged
merged 2 commits into from
Aug 26, 2024
Merged

mark analysis.pca.PCA as not parallelizable #4684

merged 2 commits into from
Aug 26, 2024

Conversation

orbeckst
Copy link
Member

@orbeckst orbeckst commented Aug 23, 2024 •
edited by github-actions bot
Loading

Fixes #4680

Changes made in this Pull Request:

  • PCA explicitly marked as not parallelizable (at least not with simple split-apply-combine)

PR Checklist

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

Developers certificate of origin

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

@orbeckst
mark analysis.pca.PCA as not parallelizable
4db6f27 
- fix #4680
- PCA explicitly marked as not parallelizable (at least not with
  simple split-apply-combine)
@pep8speaks
Copy link

pep8speaks commented Aug 23, 2024 •
edited
Loading

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

Line 389:1: E302 expected 2 blank lines, found 1
Line 407:1: W391 blank line at end of file

Comment last updated at 2024-08-26 01:24:16 UTC

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 23, 2024 •
edited
Loading

Linter Bot Results:

Hi @orbeckst! 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/10551888204/job/29229933588

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

@codecov Codecov
Copy link

codecov bot commented Aug 23, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.60%. Comparing base (94b1b79) to head (aa31e52).
Report is 1 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #4684      +/-   ##
===========================================
- Coverage    93.62%   93.60%   -0.03%     
===========================================
  Files          173      185      +12     
  Lines        21418    22485    +1067     
  Branches      3978     3978              
===========================================
+ Hits         20052    21046     +994     
- Misses         903      976      +73     
  Partials       463      463

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

hmacdope
hmacdope requested changes Aug 24, 2024
View reviewed changes
package/CHANGELOG
@@ -55,6 +55,7 @@ Fixes
Enhancements
* Introduce parallelization API to `AnalysisBase` and to `analysis.rms.RMSD` class
(Issue #4158, PR #4304)
* explicitly mark `analysis.pca.PCA` as not parallelizable (Issue #4680)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newest first

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know... but may I respectfully disagree here and suggest that we add anything regarding parallelization underneath the parent entry (Issue #4158)? It makes a lot more sense when you read the CHANGELOG.

If it's too confusing to break our convention here, I am happy to switch, of course.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries

@orbeckst orbeckst self-assigned this Aug 26, 2024
@orbeckst orbeckst merged commit d73995a into develop Aug 26, 2024
22 of 23 checks passed
@orbeckst orbeckst deleted the pca-no-parallel branch August 26, 2024 10:07
yuxuanzhuang pushed a commit to yuxuanzhuang/mdanalysis that referenced this pull request Sep 9, 2024
@orbeckst @yuxuanzhuang
mark analysis.pca.PCA as not parallelizable (MDAnalysis#4684)
dc9654e 
- fix MDAnalysis#4680
- PCA explicitly marked as not parallelizable (at least not with
  simple split-apply-combine)
- add tests
- update CHANGELOG
hmacdope pushed a commit that referenced this pull request Sep 10, 2024
@yuxuanzhuang @MattTDavies
@IAlibay @orbeckst
Remove GSD from optional test dependencies (#4707)
319b667 
* Fixed high dimensional GroupBase indexing.

* fixed pep8 issues

* Removed sanitisation

* Fix #4687 -- rdkit values in azure CI (#4688)

* Investigate rdkit issue

* Update azure-pipelines.yml

* fix numpy 2.0 import block

* fix imports

* mark analysis.pca.PCA as not parallelizable (#4684)

- fix #4680
- PCA explicitly marked as not parallelizable (at least not with
  simple split-apply-combine)
- add tests
- update CHANGELOG

* disable gsd

* disable gsd in azure

* reduce timeout and set logical

* fix azure

* restore timeout to 200

---------

Co-authored-by: Matthew Davies <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Oliver Beckstein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmacdope hmacdope hmacdope approved these changes

Assignees

@orbeckst orbeckst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MDAnalysis.analysis.pca: Implement parallelization or mark as unparallelizable
3 participants
@orbeckst @pep8speaks @hmacdope