Simplify tICA load and save

[kyleabeauchamp]

Now I'm hunting down all the places that call this...

[kyleabeauchamp]

OK I think this should do it.

[kyleabeauchamp]

Let me know if you think it's appropriate to list tICA under metrics...

[kyleabeauchamp]

Hmm, I guess it doesn't actually inherit Metric...

[kyleabeauchamp]

I'm open to thoughts on how to better expose tICA without confusing it with metrics.

[schwancr]

Yea :/ It uses the RedDimPNorm metric, which I originally named thinking that we could have a PCA thing and a ktICA thing, and a diff. map thing that all used that metric. Also that's why there's a reduce submodule with only tica in it

On Fri, Feb 7, 2014 at 4:15 PM, kyleabeauchamp notifications@github.comwrote:

Hmm, I guess it doesn't actually inherit Metric...

Reply to this email directly or view it on GitHubhttps://github.com/SimTk/msmbuilder/pull/341#issuecomment-34521370 .

[kyleabeauchamp]

OK how about I undo that and just expose msmb.reduce.tica

[schwancr]

Yea I don't think it's really a metric. Though this is confused in Cluster.py for instance as it is a metric in that sense.

[kyleabeauchamp]

OK exposed tICA via reduce

[kyleabeauchamp]

FYI so now you can just do this:

import msmbuilder as msmb
tica = msmb.reduce.tICA.load("./out.h5")

[kyleabeauchamp]

Failure can be ignored, just a timeout. I tested on my box.

[rmcgibbo]

I'm not too familiar with the tICA code, so I'll let @schwancr review/merge this.

[kyleabeauchamp]

Yeah there remains some questions about how to handle things in Cluster.py / parsers.py. It seems to me like we want to trash an entire level of parsing, but I'm not sure what Christian thinks

[kyleabeauchamp]

OK I made a possible simplification of the tICA code in parsers.py where I gave tICA first-class citizenship, rather than treating it like a plugin. Let me know if that was appropriate.

[kyleabeauchamp]

The point being that there is no longer the idea of a sub-metric, as the tica HDF file keeps track of the original metric.

[schwancr]

Oh right that's a good point, this makes the parser code much better

One related item is the fact that tICA_train.py needs to be given a metric to use to produce a vectorized representation. Having the tica metric available there might be confusing for people because there is no tica object to pass yet.

This is related to the fact that the rmsd metric is technically available at the tICA_train.py parser, but it is not possible to actually use. This speaks to the fact that we should think about moving to featurizers sometime, and we can solve this problem then.

On Fri, Feb 7, 2014 at 5:27 PM, kyleabeauchamp notifications@github.comwrote:

The point being that there is no longer the idea of a sub-metric, as the tica HDF file keeps track of the original metric.

Reply to this email directly or view it on GitHubhttps://github.com/SimTk/msmbuilder/pull/341#issuecomment-34524276 .

[kyleabeauchamp]

OK hopefully I've finished the cleanup.

[kyleabeauchamp]

Alright I think this is good to go after some review.

[schwancr]

This looks good, but one thing that we could change is to make the load method a class method on the tICA object, then we avoiding having to do tICA.tICA like we did with Trajectory.

Want to do this? I can make the change if you're busy

[kyleabeauchamp]

Done

[schwancr]

Does this pass tests? It looks like you didn't modify tICA_train.py, which will do this:

arglib.ArgumentParser(get_basic_metric=True)

That functionality should go away now, since there is no such thing as a "layer" metric.

[schwancr]

So arglib.py needs changes to remove these lines: https://github.com/SimTk/msmbuilder/blob/master/src/python/arglib.py#L140-L143

And tICA_train.py needs to change to not use the kwarg introduced above.

[kyleabeauchamp]

Yeah, I didn't want to change things too dramatically because I wasn't sure whether other people had plans for the metric parsers stuff.

[kyleabeauchamp]

e.g. KtICA

[schwancr]

I think it can be treated the same as tICA, since it can just pickle.dumps the underlying metric.

On Tue, Feb 11, 2014 at 11:35 AM, kyleabeauchamp notifications@github.comwrote:

e.g. KtICA

Reply to this email directly or view it on GitHubhttps://github.com/SimTk/msmbuilder/pull/341#issuecomment-34796114 .

[kyleabeauchamp]

OK I think I got rid of the remnants of the "basic" metric

[kyleabeauchamp]

tests pass

[schwancr]

This looks fine to me.

One potential point of messiness caused by the "layer metric" thing is that the add_basic_metric_parsers function could easily be placed in the add_matric_parsers function

[kyleabeauchamp]

OK I merged the add_metric_parsers and add_basic_metric_parsers functions.

[kyleabeauchamp]

tests pass on my box...

[kyleabeauchamp]

OK I'm going to merge if there are no further wishes

[schwancr]

Why did travis fail?

[rmcgibbo]

Just look and scroll down. It timed out on cluster_hierarchical, which it is prone to do sometimes.

[schwancr]

Alright, let me rephrase my question:

"should we care that travis failed?"

[rmcgibbo]

No.

On Wed, Feb 12, 2014 at 12:31 PM, Christian Schwantes < notifications@github.com> wrote:

Alright, let me rephrase my question:

"should we care that travis failed?"

Reply to this email directly or view it on GitHubhttps://github.com/SimTk/msmbuilder/pull/341#issuecomment-34913897 .

[kyleabeauchamp]

I just ran these commands without issue:

nosetests

cd Tutorial

tICA_train.py -d 1 -o out.h5 atompairs
Cluster.py tica -f out.h5 -n 3 kcenters -k 50
 rm Data/Assignments.h5*
Assign.py -g Data/Gens.h5 tica -f out.h5 -n 3