[WIP] Issue 785 add auxiliary #868

fiona-naughton · 2016-06-03T21:51:25Z

WIP for #785.

Currently, allows adding data from an xvg file to a trajectory by add_auxiliary(filename, name, **kwargs). When reading the next trajectory timestep, the auxiliary timesteps closest to that trajectory timestep are read through, and a 'representative' value calculated from these and stored in the timestep in an aux namespace, as ts.aux.name. The 'representative' value can be either an average or the value from the closest timepoint (with an optional cutoff).

The XVGReader itself also has go_to_step and go_to_ts methods for jumping to specified auxiliary steps/trajectory timesteps; the latter's used to bring the auxiliary to the same time as the trajectory when it's first added, but I've not hooked it up with the equivalent trajectory method yet.

There's a couple things that seem to work but I'm not sure I've gone about the right way (particularly the go_to methods; they currently just start reading form the top of the file and wait till the times match up...); apologies if I'm doing anything particularly silly!

TODO

Expand to other filetypes + implement a guess_reader
Allow to select columns/name individually when reading multi-column data?
Dealing with 'missing data'?
Allow to iterate with auxiliary dt, rather than trajectory's
Proper testing!

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

richardjgowers · 2016-06-06T08:13:47Z

package/MDAnalysis/coordinates/base.py

@@ -216,6 +224,9 @@ def __init__(self, n_atoms, **kwargs):
        self.has_forces = kwargs.get('forces', False)

        self._unitcell = self._init_unitcell()
+
+        # set up aux namespace for adding auxiliary data
+        self.aux = Namespace()


I think you can just replace this with a simple dictionary

It depends on how we want to access the auxiliary data. In all the discussions we assumed something like ts.aux.force; this requires a class as what @fiona-naughton used. ts.aux['force']is fine by me.

I actually would prefer the first ts.aux.force because I can TAB complete very easily in an interactive session. (yeah I'm lazy)

+1 for the Namespace solution. (tab-completion plus I'm lazy to type square brackets)

Yeah, I like it with the Namespace; it doesn't make much difference, but it's that little bit easier to type! (and it's nice to be lazy sometimes >_< )

richardjgowers · 2016-06-06T08:23:34Z

Looks good. With the xvgreader, you can probably get away with reading the entire file at once, then seeking is just finding places in the numpy array that stores it.

That said, all that is just an implementation detail, get the tests up and running as they will mostly stay the same as you keep working on this. These can (and should) be very simple, eg you could have an xvg file with just [1, 2, 3] in it, then check that reading this gives you 1, 2, 3 again.

richardjgowers · 2016-06-06T08:38:02Z

package/MDAnalysis/auxiliary/xvg.py

+        ## the auxreader is the first step 'belonging' of the next ts...
+
+    def reset_ts(self, ts):
+        self.ts_data = np.array([])


Here you're passing in ts and not using it?

mnmelo · 2016-06-06T09:32:51Z

I just added a comment/question in #785 to expand on @richardjgowers' idea of reading data all at once.

I raised this also because the current purpose of the AuxReader base class is a bit unclear to me. It seems to be a file-agnostic class, suitable for a read-all-at-once model. However, it has references to self.reopen(), which point to it being more of an abstract class for always-read-from-file subclasses.

richardjgowers · 2016-06-06T18:59:31Z

Ok so now the PR you did has gone through, you need to update your local version of develop (fetch and pull) and then merge develop into this branch to get the changes you recently made into this branch. You might get a merge conflict on files that changed in each branch, you just need to go over it carefully maybe using something like meld or diffuse.

kain88-de · 2016-06-06T19:40:37Z

merge develop into this branch to get the changes you recently made into this branch

Please to a rebase instead of a merge. It keeps a cleaner history and it is easier to spot when the git-merge conflict solver does unintended things (removed code can reappear or added code can be deleted by accident!)

fiona-naughton · 2016-06-06T21:46:34Z

Ah, just merged it sorry (but doesn't look like it even worked completely... I'm using GitHub desktop at the moment, and not entirely sure what it's done?). Might leave it to figure out tomorrow when I'm back at work and not on Windows...

jbarnoud · 2016-06-09T07:14:49Z

It seems that you broke something with Timesteps. Multiple tests fail with the following traceback:

======================================================================
ERROR: MDAnalysisTests.coordinates.test_dlpoly.TestDLPolyHistoryMinimal.test_getting
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/MDAnalysisTests/coordinates/test_dlpoly.py", line 123, in test_getting
    assert_equal(ts.frame, 1)
AttributeError: 'NoneType' object has no attribute 'frame'

Since different readers are broken in the same way, the breakage most likely happened in the ProtoReader or the base Reader class that get to update ts.frame wrongly. I do not see which of your changes could cause that, though.

jbarnoud · 2016-06-09T07:18:57Z

package/MDAnalysis/coordinates/base.py

+class Namespace(object):
+    # set up a basic class so we can make an 'aux' namespace in Timestep 
+    # for auxiliary data; might be better defined elsewhere...?
+    # There's probably a better way to do this


Seems to me as the good way and the good place to do this, as there seems to be a consensus around the namespace vs the dictionary. The class can be moved latter to .lib.util if it get duplicated.

coveralls · 2016-06-09T17:02:19Z

Coverage decreased (-0.8%) to 79.501% when pulling de25b17 on fiona-naughton:issue-785-add-auxiliary into debae2b on MDAnalysis:develop.

fiona-naughton · 2016-06-09T17:33:20Z

I've made a bunch of changes, largely to make the matching of auxiliary steps to trajectory timesteps a little nicer (I think) and hopefully clearer. There's now a step_to_frame(i, ts) method to return the frame number an auxiliary step i will be considered part of; I'm now using this for the current/previous/next step number to check both if we're starting at the right spot and whether to keep reading to the next step while reading a timestep (in place of step_in_ts and first_in_ts). It also means the current auxiliary step after reading a timestep will be the last step in that timestep, rather than the first in the next one. Hopefully that makes more sense!

(The failed tests were due to my forgetting to add a return line; it's fixed now. I'll finish properly writing up some tests for the new stuff and hopefully add that soon)

jbarnoud · 2016-06-10T07:32:18Z

package/MDAnalysis/auxiliary/base.py

+    When reading in a trajectory, auxiliary data steps are 'assigned' the 
+    closest trajectory timestep.
+
+    Attributes:


Should be:

Attributes ==========

to follow numy doctring guidelines.

…thods

…imestep

…election; assort fixes

…d error-raises when options invalid

…Namespace to Utils

Optimise perf from use of range in auxiliaries

fix step_to_frame when trajectory starts after 0

… various renaming and doc fixes

…ux take into account cutoff tidy docs + tests

fiona-naughton · 2016-08-16T16:59:23Z

Rebased + had a go at squashing some of the commits - hopefully nothing went disastrously wrong and it still makes sense!

coveralls · 2016-08-16T17:28:35Z

Changes Unknown when pulling a9e067d on fiona-naughton:issue-785-add-auxiliary into * on MDAnalysis:develop*.

jbarnoud · 2016-08-17T12:33:15Z

Youhou! It is finally merged! Congrats!

orbeckst · 2016-08-18T17:28:56Z

@fiona-naughton congratulations! Thank you for this exciting new feature and your participation in GSoC! I hope we see more of you, even after the end of GSoC.

…uxiliary [WIP] Issue 785 add auxiliary

richardjgowers reviewed Jun 6, 2016
View reviewed changes

kain88-de added enhancement Component-Readers Work in progress labels Jun 6, 2016

kain88-de added this to the 0.16.0 milestone Jun 6, 2016

mnmelo mentioned this pull request Jun 6, 2016

Adding auxiliary information to trajectory #785

Closed

fiona-naughton force-pushed the issue-785-add-auxiliary branch from d3bc64a to 3875179 Compare June 6, 2016 17:42

fiona-naughton force-pushed the issue-785-add-auxiliary branch from 9de9ab3 to 77cf750 Compare June 7, 2016 11:25

jbarnoud reviewed Jun 9, 2016
View reviewed changes

jbarnoud reviewed Jun 10, 2016
View reviewed changes

fiona-naughton and others added 20 commits August 16, 2016 17:26

first xvgreader attempt

0fc664f

add aux namespace to timestep and give reader add/remove auxiliary me…

a947f67

…thods

add auxreader time/dt/etc attributes + method to move to particular t…

11e1c14

…imestep

update auxiliaries with trajectory change by next() or jump

3d80851

aux steps now 0-based, store n_steps + time for each; allow columns s…

42e1782

…election; assort fixes

add next/iter as aux

a692f06

add tests; add auxreader guesser; update Namespace; fix defaults + ad…

baf10b0

…d error-raises when options invalid

fixes for iter/next as aux; force aux rewind in PDB + XDR readers

03c279f

add a read-all-to-memory xvg reader; update tests+docs

e60a247

add methods for recording AuxReader info for reloading

e453c31

generalise time/data selection beyond column; various renaming; move …

2d5f411

…Namespace to Utils

add a AuxStep class

fdb81d0

add __getitem__ for auxreader; assort. small fixes

302f832

add getitem to Namespace

cb3f88f

tidy + various fixes (AuxReader __getitem__, AuxStep data, tests)

6d17219

Optimise perf from use of range in auxiliaries

remove extra aux bit from XDR/PDB readers

15754f3

stop at first '&' + deal with blank lines in xvg reader

f0dc612

fix auxreader slice so last step is included

0474544

fix step_to_frame when trajectory starts after 0

make default time/data selector; add a guesser to return an instance;…

99b0419

… various renaming and doc fixes

methods for accessing aux attributes from traj reader; iter/next as a…

5cf376a

…ux take into account cutoff tidy docs + tests

fiona-naughton force-pushed the issue-785-add-auxiliary branch from f6d7d98 to a9e067d Compare August 16, 2016 16:57

update changelog

a9e067d

jbarnoud merged commit 06c3e14 into MDAnalysis:develop Aug 17, 2016

jbarnoud self-assigned this Aug 17, 2016

jbarnoud removed the Work in progress label Aug 17, 2016

abiedermann pushed a commit to abiedermann/mdanalysis that referenced this pull request Jan 5, 2017

Merge pull request MDAnalysis#868 from fiona-naughton/issue-785-add-a…

133973d

…uxiliary [WIP] Issue 785 add auxiliary

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Issue 785 add auxiliary #868

[WIP] Issue 785 add auxiliary #868

fiona-naughton commented Jun 3, 2016 •

edited by kain88-de

Loading

richardjgowers Jun 6, 2016

jbarnoud Jun 8, 2016

kain88-de Jun 8, 2016

mnmelo Jun 8, 2016

fiona-naughton Jun 8, 2016

richardjgowers commented Jun 6, 2016

richardjgowers Jun 6, 2016

mnmelo commented Jun 6, 2016

richardjgowers commented Jun 6, 2016

kain88-de commented Jun 6, 2016

fiona-naughton commented Jun 6, 2016

jbarnoud commented Jun 9, 2016

jbarnoud Jun 9, 2016

coveralls commented Jun 9, 2016 •

edited

Loading

fiona-naughton commented Jun 9, 2016

jbarnoud Jun 10, 2016

fiona-naughton commented Aug 16, 2016

coveralls commented Aug 16, 2016 •

edited

Loading

jbarnoud commented Aug 17, 2016

orbeckst commented Aug 18, 2016

[WIP] Issue 785 add auxiliary #868

[WIP] Issue 785 add auxiliary #868

Conversation

fiona-naughton commented Jun 3, 2016 • edited by kain88-de Loading

PR Checklist

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

richardjgowers commented Jun 6, 2016

Choose a reason for hiding this comment

mnmelo commented Jun 6, 2016

richardjgowers commented Jun 6, 2016

kain88-de commented Jun 6, 2016

fiona-naughton commented Jun 6, 2016

jbarnoud commented Jun 9, 2016

Choose a reason for hiding this comment

coveralls commented Jun 9, 2016 • edited Loading

fiona-naughton commented Jun 9, 2016

Choose a reason for hiding this comment

fiona-naughton commented Aug 16, 2016

coveralls commented Aug 16, 2016 • edited Loading

jbarnoud commented Aug 17, 2016

orbeckst commented Aug 18, 2016

fiona-naughton commented Jun 3, 2016 •

edited by kain88-de

Loading

coveralls commented Jun 9, 2016 •

edited

Loading

coveralls commented Aug 16, 2016 •

edited

Loading