MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.28k stars 646 forks source link

nuclinfo module does not use AnalysisBase #3600

Closed orbeckst closed 2 years ago

orbeckst commented 2 years ago

Expected behavior

Analysis modules follow standard behavior, i.e., use AnalysisBase when possible.

Actual behavior

nuclinfo consists of legacy code that predates AnalysisBase.

Current version of MDAnalysis

orbeckst commented 2 years ago

To address this issue I suggest to

orbeckst commented 2 years ago

This is not a small issue. Depending on how difficult it becomes in practice, we can also do PRs for individual functions, as soon as the basic skeleton (new module, one new analysis class, corresponding deprecation) has been established.

ALescoulie commented 2 years ago

I'll get started on this. I've seen the way speed was improved in dihedral.

olivia632 commented 2 years ago

i want to work on this can you help me with the pull request theoretically its confusing

olivia632 commented 2 years ago

i have done the fork part I am not able to do make the pull request if you could help me in what should the base rach and compared branch to be

ALescoulie commented 2 years ago

@orbeckst I've been looking at nucinfo.py, and I have a question about what you want. If the user provides a list their own selections why have separate classes for wc_dist, minor_dist, and major_dist since the only difference in each function is the atoms selected in the base pair. Could I just make a single class for getting distances of a list of selections then in the docs mention how it can be used to get the different distances? What I could do is just write a NucPairDist class that is more optmized then have more specific subclasses running the individual calculations.

orbeckst commented 2 years ago

i have done the fork part I am not able to do make the pull request if you could help me in what should the base rach and compared branch to be

The base branch is develop in the https://github.com/MDAnalysis/mdanalysis repo, the head is the branch in your fork where you're adding new code.

orbeckst commented 2 years ago

If the user provides a list their own selections why have separate classes for wc_dist, minor_dist, and major_dist since the only difference in each function is the atoms selected in the base pair. Could I just make a single class for getting distances of a list of selections then in the docs mention how it can be used to get the different distances?

Having a generic class is excellent. Just having the docs describing how to get the effect makes life more difficult for users.

What I could do is just write a NucPairDist class that is more optmized then have more specific subclasses running the individual calculations.

I like the approach of thin classes that only define the default selection. They can then serve as examples how to do the analysis for one FF and we can eventually extend them to recognize others if necessary.

IAlibay commented 2 years ago

i have done the fork part I am not able to do make the pull request if you could help me in what should the base rach and compared branch to be

@olivia632 questions re: using git & GitHub would be best handled either on our mailing list or discord. If you could ask these there that would be very much appreciated. The main thing is that this can easily lead to long conversations which are not directly linked to the issue being described.

IAlibay commented 2 years ago

Just a note here for all potential GSoC contributors - we do operate on a "first PR basis", but this is a pretty big proposed change. I would encourage that potential contributors try to be good citizen to each other and work out the best way for multiple interested parties to not end up spending hours writing the same code.