pmda icon indicating copy to clipboard operation
pmda copied to clipboard

Published 20 hours ago • verified publisher icon MDAnalysis

Refactor pmda after universe can be serialized

Open yuxuanzhuang opened this issue 4 years ago • 8 comments

Fixes #133

Changes made in this Pull Request:

  • refactor each part of pmda (test passed)
    • [x] parallel.py
    • [x] custom.py
    • [x] rmsd
    • [x] rmsf
    • [x] contact
    • [x] Hbond
    • [x] RDF
    • [x] density
    • [ ] leaflet

PR Checklist

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

yuxuanzhuang avatar Jul 15 '20 12:07 yuxuanzhuang

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

pep8speaks avatar Jul 15 '20 12:07 pep8speaks

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 avatar Jul 15 '20 18:07 orbeckst

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.

orbeckst avatar Jul 20 '20 19:07 orbeckst

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.

yuxuanzhuang avatar Aug 09 '20 16:08 yuxuanzhuang

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.

orbeckst avatar Aug 09 '20 22:08 orbeckst

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 avatar Aug 10 '20 09:08 yuxuanzhuang

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

yuxuanzhuang avatar Aug 19 '20 11:08 yuxuanzhuang

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.

VOD555 avatar Aug 23 '20 00:08 VOD555