Refactor align #1136

kain88-de · 2016-12-22T20:35:29Z

It stills needs some checking and tests. I would like go get this into 0.16.0 though. The RMSD class has been refactored this cycle and I'm currently not sure if the bug has been present as well in 0.15.0

Changes made in this Pull Request:

use arbitrary weights in align.py and rms.py
rewrite RMSF to use AnalysisBase
fix centering bug in RMSD (should be done with chosen weights)

PR Checklist

Tests?
tests for new deprecated option
Docs?
CHANGELOG updated?
~~[ ] Issue raised/referenced?~~
add deprecation warnings for mass_weighted
updated functions depending on new kwarg
fix all mass_weighted in encore. Or at least raise an issue and fix in other PR

kain88-de · 2016-12-29T10:03:09Z

@mtiberti @wouterboomsma I see that encore also uses the mass_weighted argument. Would it be hard to change it to weights='mass' to allow for arbitrary weights like I introduce here in the PR for rms and alignment function?

kain88-de · 2016-12-29T10:15:20Z

Ok So i'm think I'm done with this. All the internal references to the old keyword have been replaced and appropriate deprecation messages are raised right now when the old one is used. Only the encore module still use the old keyword. I haven't looked yet at how hard it would be to change them as well.

kain88-de · 2016-12-29T10:15:50Z

I haven't looked into actually using arbitrary weights in the tests. I will still do that.

kain88-de · 2016-12-30T22:45:52Z

@MDAnalysis/coredevs can anyone see why the travis build fails. For some reason nosetest doesn't exit with 0.

tylerjereddy · 2016-12-30T23:08:09Z

testsuite/MDAnalysisTests/analysis/test_rms.py

+            warnings.simplefilter('always')
+            RMSD = MDAnalysis.analysis.rms.RMSD(self.universe, select='name CA',
+                                                step=49, mass_weighted=True).run()
+        assert(len(warn), 1)


Travis CI build seems to whine about this with testsuite/MDAnalysisTests/analysis/test_rms.py:203: [W0199(assert-on-tuple), TestRMSD.test_mass_weighted_and_save_deprecated] Assert called on a 2-uple. Did you mean 'assert x,y'?

thanks. I meant assert_equal

kain88-de · 2016-12-31T13:29:56Z

OK I'm done now this PR. Someone could please review this. Their is still the the mass_weighted kwarg in encore. But to remove that I will do some refactoring independent of this PR to simplify the code.

kain88-de · 2016-12-31T14:39:16Z

How can I find out in the coverage API which lines I have missed now with the changes here.

richardjgowers · 2017-01-01T15:07:18Z

https://coveralls.io/builds/9478683 this shows the current coverage for this commit. coveralls doesn't let you see the delta in lines caused by this PR though, (I think codecov does though)

orbeckst

Overall looking clean. Thanks for updating RMSF!

See comments/questions.

I didn't have time to review the tests.

orbeckst · 2017-01-11T01:10:33Z

package/MDAnalysis/analysis/align.py


    Returns
    -------
    old_rmsd
        RMSD before spatial alignment
    new_rmsd
-        RMSD after spatial alignment.


Why did you remove the description?

orbeckst · 2017-01-11T01:10:49Z

package/MDAnalysis/analysis/align.py

@@ -404,6 +405,9 @@ def alignto(mobile, reference, select="all", mass_weighted=False,
       Uses :func:`get_matching_atoms` to work with incomplete selections
       and new *strict* keyword. The new default is to be lenient whereas
       the old behavior was the equivalent of *strict* = ``True``.
+
+    .. versionchanged:: 0.16.0
+       new general 'weights' kwarg replace mass_weights


add deprecation for mass_weighted kw

orbeckst · 2017-01-11T01:14:57Z

package/MDAnalysis/analysis/align.py

+       When using 2. or 3. with *sel1* and *sel2* then these selections can
+       also each be a list of selection strings (to generate a AtomGroup with
+       defined atom order as described under :ref:`ordered-selections-label`).
+    mass_weighted : boolean, optional, deprecated


below you wrote this as `(deprecated) – consistency?

orbeckst · 2017-01-11T01:17:29Z

package/MDAnalysis/analysis/encore/covariance.py

@@ -173,41 +173,34 @@ def shrinkage_covariance_estimator( coordinates,
 def covariance_matrix(ensemble,
                      selection="name CA",
                      estimator=shrinkage_covariance_estimator,
-                      mass_weighted=True,
+                      weights=None,


This should be "mass" to retain the previous default behavior.

orbeckst · 2017-01-11T01:20:28Z

package/MDAnalysis/analysis/encore/covariance.py

+                req_len = ensemble.select_atoms(selection).n_atoms
+            else:
+                req_len = ensemble.atoms.n_atoms
+            if req_len != len(weights):


Isn't this going to fail when weights == None?

This code path can only be reached if if weights above evaluates to true

orbeckst · 2017-01-11T01:22:28Z

package/MDAnalysis/analysis/rms.py

-        relative_weights = relative_weights.astype(np.float64)
-    else:
-        relative_weights = None
-
    # superposition only works if structures are centered
    if center or superposition:
        # make copies (do not change the user data!)
        # weights=None is equivalent to all weights 1
        a = a - np.average(a, axis=0, weights=weights)


Not your PR but nicer would be

a -= np.average(a, axis=0, weights=weights)

orbeckst · 2017-01-11T01:24:01Z

package/MDAnalysis/analysis/rms.py

@@ -335,8 +330,10 @@ def __init__(self, atomgroup, reference=None, select='all',

        filename : str (optional)
            write RSMD into file file :meth:`RMSD.save`
-        mass_weighted : bool (optional)
+        mass_weighted : bool deprecated


(deprecated) ?

orbeckst · 2017-01-11T01:27:15Z

package/MDAnalysis/analysis/rms.py

-        self._rmsf = rmsf
-
-    @property
-    def rmsf(self):


Having removed RMSF.rmsf should be mentioned in a versionchanged 0.16.0 doc, together with the transition to standard analysis API.

This isn't removed. It can be read the same way as before. Just that you can now also write to the value. So there isn't a change for the user. We handle all analysis classes right now that computed values can also be overwritten. So this is just making it behave more like the rest of MDAnalysis.

kain88-de · 2017-01-11T08:27:45Z

The final changes of this depend on #1145 to update encore as well to the new keyword.

orbeckst · 2017-01-15T23:09:33Z

@kain88-de , I had some minor comments on the docs (please have a look) and the question of default values, to which you replied #1136 (comment)

The final changes of this depend on #1145 to update encore as well to the new keyword.

Now that #1145 is done, what should the defaults for mass weighting be?

kain88-de · 2017-01-16T06:49:00Z

@orbeckst thanks for your comments. The default that I changed was to follow the function docs, if I remember correct. The default value for encore is a good question. @wouterboomsma and @mtiberti what weights would you prefer for encore as default masses or none? I'd appreciate input from you anyway with regard to the new feature to choose arbitrary weights.

wouterboomsma · 2017-01-17T15:18:30Z

@kain88-de Very nice work on supporting arbitrary weights. It should be easy to support this in the encore front-end as well. I think @mtiberti will just look into this. Regarding the default values, the current default is mass_weighted=True, so I guess we should set it to weights='mass' now?

simplify weights handling

This new general weights kwarg can take an mass argument or arbitrary weights to allow new fitting algorithms.

This also fixes a centering bug. Previous the center of mass was used independent of the used weights. Now we center with the chosen weights. This means results might change!

The old results have been wrong due to a centering bug in the RMSD code

This changes the methods in encore to the new arbitrary weights system. The last one conf_distance_matrix does need more refactoring work to simplify the code before I change it as well.

This replaces the old mass only weights with general weights

kain88-de · 2017-02-04T08:37:58Z

Superseeded by #1197

kain88-de added Component-Analysis refactoring Work in progress labels Dec 22, 2016

kain88-de force-pushed the refactor-align branch 2 times, most recently from b358b78 to 01bf8b2 Compare December 29, 2016 09:56

kain88-de removed the Work in progress label Dec 29, 2016

kain88-de force-pushed the refactor-align branch from c8a6ecd to 1c7c671 Compare December 30, 2016 20:55

tylerjereddy reviewed Dec 30, 2016

View reviewed changes

kain88-de force-pushed the refactor-align branch from 8c324e7 to a07fcae Compare December 31, 2016 13:28

orbeckst requested changes Jan 11, 2017

View reviewed changes

kain88-de mentioned this pull request Jan 12, 2017

Refactor encore conformational_distance_matrix #1145

Merged

3 tasks

kain88-de added 8 commits January 27, 2017 19:16

refactor rms.py:rmsd

12b65f3

simplify weights handling

Add weights kwarg to alignto

69a6dbf

This new general weights kwarg can take an mass argument or arbitrary weights to allow new fitting algorithms.

introduce weights kwargs also for AlignTrj

729d8bc

RMSD uses new weights kwarg, fixes bug as well

be6dd77

This also fixes a centering bug. Previous the center of mass was used independent of the used weights. Now we center with the chosen weights. This means results might change!

correct test results!

4f8c5be

The old results have been wrong due to a centering bug in the RMSD code

convert RMSF to AnalysisBase

758b43d

fix docs from using deprecated method

9959db7

add deprecation warnings

49eb5a5

kain88-de added 4 commits January 27, 2017 19:16

update CHANGELOG

0c19152

add deprecation tests

9477de7

Change encore function expect conf distance

0299873

This changes the methods in encore to the new arbitrary weights system. The last one conf_distance_matrix does need more refactoring work to simplify the code before I change it as well.

fix docs and default parameters

cffa207

kain88-de force-pushed the refactor-align branch from a07fcae to cffa207 Compare January 27, 2017 21:28

kain88-de added 2 commits February 4, 2017 09:32

update confdistmatrix.py to new weights

11d4d46

This replaces the old mass only weights with general weights

update tests to new kwargs

937c523

kain88-de mentioned this pull request Feb 4, 2017

Refactor align #1197

Merged

8 tasks

kain88-de closed this Feb 4, 2017

kain88-de deleted the refactor-align branch April 10, 2017 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor align #1136

Refactor align #1136

kain88-de commented Dec 22, 2016 •

edited

Loading

kain88-de commented Dec 29, 2016

kain88-de commented Dec 29, 2016

kain88-de commented Dec 29, 2016

kain88-de commented Dec 30, 2016

tylerjereddy Dec 30, 2016

kain88-de Dec 31, 2016

kain88-de commented Dec 31, 2016

kain88-de commented Dec 31, 2016

richardjgowers commented Jan 1, 2017

orbeckst left a comment

orbeckst Jan 11, 2017

orbeckst Jan 11, 2017

orbeckst Jan 11, 2017

orbeckst Jan 11, 2017

orbeckst Jan 11, 2017

kain88-de Jan 11, 2017

orbeckst Jan 12, 2017

orbeckst Jan 11, 2017

orbeckst Jan 11, 2017

orbeckst Jan 11, 2017

kain88-de Jan 11, 2017

orbeckst Jan 12, 2017

kain88-de commented Jan 11, 2017

orbeckst commented Jan 15, 2017

kain88-de commented Jan 16, 2017

wouterboomsma commented Jan 17, 2017

kain88-de commented Feb 4, 2017

Refactor align #1136

Refactor align #1136

Conversation

kain88-de commented Dec 22, 2016 • edited Loading

PR Checklist

kain88-de commented Dec 29, 2016

kain88-de commented Dec 29, 2016

kain88-de commented Dec 29, 2016

kain88-de commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de commented Dec 31, 2016

kain88-de commented Dec 31, 2016

richardjgowers commented Jan 1, 2017

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de commented Jan 11, 2017

orbeckst commented Jan 15, 2017

kain88-de commented Jan 16, 2017

wouterboomsma commented Jan 17, 2017

kain88-de commented Feb 4, 2017

kain88-de commented Dec 22, 2016 •

edited

Loading