Secondary structure determination#2609
Secondary structure determination#2609lilyminium wants to merge 7 commits intoMDAnalysis:developfrom
Conversation
lilyminium
force-pushed
the
dssp
branch
2 times, most recently
from
29f64ef to
09fd9af
Compare
|
@lilyminium this will be a cool addition. WRT installation, a better long term solution is maybe to create a conda package for these tools. Also I'm remembering something about one of these tools having a different license that we have to be careful about? |
|
@richardjgowers is it STRIDE?
DSSP uses:
MDTraj is LGPL. |
Codecov Report
@@ Coverage Diff @@
## develop #2609 +/- ##
===========================================
- Coverage 90.61% 87.64% -2.98%
===========================================
Files 174 179 +5
Lines 23554 24585 +1031
Branches 3072 3219 +147
===========================================
+ Hits 21343 21547 +204
- Misses 1585 2409 +824
- Partials 626 629 +3
Continue to review full report at Codecov.
|
| @@ -0,0 +1,2415 @@ | |||
| # -*- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding:utf-8 -*- | |||
There was a problem hiding this comment.
Why is there a copy of coordinates/base.py in convertors?
There was a problem hiding this comment.
Oops, mixed up my branches.
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Having secondary structure determination is great.
I am somewhat ambivalent about falling back to mdtraj's dssp implementation. Their LGPL license allows us to import their code but vice versa they would not be able to do this, so even though this is perfectly legal it feels a bit like taking from a competitor without giving anything back. Perhaps if we also had a converter for mdtraj objects then there would be more of an interoperabilty angle. @MDAnalysis/coredevs , do you have an opinion on how we should proceed?
| - BUILD_CMD="pip install -e package/ && (cd testsuite/ && python setup.py build)" | ||
| - CONDA_MIN_DEPENDENCIES="mmtf-python mock six biopython networkx cython matplotlib scipy griddataformats hypothesis gsd codecov" | ||
| - CONDA_DEPENDENCIES="${CONDA_MIN_DEPENDENCIES} seaborn>=0.7.0 clustalw=2.1 netcdf4 scikit-learn joblib>=0.12 chemfiles" | ||
| - CONDA_DEPENDENCIES="${CONDA_MIN_DEPENDENCIES} seaborn>=0.7.0 clustalw=2.1 netcdf4 scikit-learn joblib>=0.12 chemfiles mdtraj" |
There was a problem hiding this comment.
Will a user have to install mdtraj in order to use MDAnalysis? I'd like to avoid this situation.
There was a problem hiding this comment.
No, it raises an ImportError only when the class is instantiated.
| n_name='N', ca_name='CA', pro_name='PRO'): | ||
| # TODO: implement this on its own w/o mdtraj? | ||
| try: | ||
| from mdtraj.geometry._geometry import _dssp |
There was a problem hiding this comment.
Would it be worthwhile to copy the code and include it here (under LGPL) and of course leave all the citations?
There was a problem hiding this comment.
I suppose so, if this is better than importing MDTraj?
| cite_module=True) | ||
|
|
||
|
|
||
| class DSSP(SecondaryStructureBase): |
There was a problem hiding this comment.
I would change the name to DSSPmdtraj or something else. You can then still derive DSSP(DSSPmdtraj) but in case at some point we write our own version or bundle it then we can easily switch DSSP over.
|
As discussed with @lilyminium, given the large dependencies being added here, the plan would be to move it downstream to a "structure analysis" package which we have planned as part of the upcoming MDAKits work. |
4 participants
Fixes #2608
Changes made in this Pull Request:
secondary_structuremoduleLet’s see how these installation scripts go...
PR Checklist
Demo notebook here.