search

MDAnalysis / pmda

Parallel algorithms for MDAnalysis
https://www.mdanalysis.org/pmda/
Other
31 stars 22 forks source link

Refactor pmda after universe can be serialized #132

Open yuxuanzhuang opened 4 years ago

yuxuanzhuang commented 4 years ago

Fixes #133

Changes made in this Pull Request:

PR Checklist

pep8speaks commented 4 years ago

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

Line 16:80: E501 line too long (84 > 79 characters) Line 16:84: W504 line break after binary operator Line 58:80: E501 line too long (104 > 79 characters) Line 69:80: E501 line too long (115 > 79 characters)

Comment last updated at 2021-05-12 17:44:51 UTC
orbeckst commented 4 years ago

To get the tests going, change Travis to build and install MDAnalysis from yuxuanzhuang:serialize_io in PR https://github.com/MDAnalysis/mdanalysis/pull/2723 – there's a pip command line/url way to directly use a git branch. I think we used it for PMDA in the past.

orbeckst commented 4 years ago

You'll also have to update PMDA docs and setup.py to say that this requires MDA 2.0.0 and therefore ≥ python 3.6.

There's a question if we want to also do a PMDA 1.0 with the old MDA 1.0 and then PMDA 2.0 to be in sync with MDA 2.0.

yuxuanzhuang commented 4 years ago

I have a question regrading starting a PR based on this PR...is it possible? (a quick search indicates it's not possible in github) The reason is that the other PR (introducing dask mixin) is still experimental; I opt to separate that from this one.

orbeckst commented 4 years ago

I think you can do a PR that is relative to this one and that would be merged into this one. Check the settings for base branch when you create a new PR.

yuxuanzhuang commented 4 years ago

I think the problem is this branch is not under MDAnalysis but my private one, so that PR will be created under my own repo.

yuxuanzhuang commented 4 years ago

I disabled DeprecationWarning in this PR temporarily.

The failed test in rdf_s here seems to be related to the discrepancy between PR https://github.com/MDAnalysis/mdanalysis/pull/2812 and https://github.com/MDAnalysis/pmda/pull/121

VOD555 commented 4 years ago

It's very cool that this PR helps get rid of rebuilding the universe, and make the code much simpler.

Yeah, the test failed as we changed the definition of the option density in MDAnalysis PR, but didn't do it in PMDA.