bowman-lab / enspara

Modeling molecular ensembles with scalable data structures and parallel computing
https://enspara.readthedocs.io
GNU General Public License v3.0
33 stars 16 forks source link

Paths msmbuilder tpt #208

Closed lgsmith closed 3 years ago

lgsmith commented 3 years ago

I took the tpt.path code that enspara was calling from the msmbuilder codebase and added it into the path.py source file in the enspara code base. Although it appears to work, I've not tried to use this code in a project. It will remove msmbuilder as a dependency for setting up enspara, which is the primary reason I did this.

lgsmith commented 3 years ago

I've realized there is another reference to MSMBuilder I haven't expunged here yet, which is in _get_distance_method (it uses msmbuilder's libdistance to cover metrics that are not 'rmsd' or 'euclidean'. I'm afraid I'll have to try looking at that to see if it can be transplanted as well. Sorry to have sent this along prematurely.

justinrporter commented 3 years ago

Sorry to have sent this along prematurely.

No trouble at all! Thanks for your time and contribution (in advance)!

cover metrics that are not 'rmsd' or 'euclidean'.

Yes, I noticed that. I am not sure if anybody actually uses these. And, actually, I it's structured so you don't need the import unless you go down that branch. You could change the condition from something like:

    if metric == 'rmsd':
        return md.rmsd
    if metric == 'euclidean':
        return euclidean
    elif isinstance(metric, str):
        try:
            import msmbuilder.libdistance as libdistance
        except ImportError:
            raise ImproperlyConfigured()

to something like

    if metric == 'rmsd':
        return md.rmsd
    if metric == 'euclidean':
        return euclidean
    if metric in list_of_msmb_metrics:
        try:
            import msmbuilder.libdistance as libdistance
        except ImportError:
            raise ImproperlyConfigured("you need msmb, it's an optional dependency")

And then msmbuilder would truly be an optional dependency.

Alternatively, you could implement/copy-paste the missing distances from msmbuilder.

Alternatively alternatively, I don't think anybody uses the more exotic distance metrics anyway, I think it would probably be fine if we deprecated them until/unless someone complains.

lgsmith commented 3 years ago

These changes have been made. I wrote a more verbose help message in the libdistance error, but it is substantially similar to what you recommended. I'll try to submit the time pr next. It'll depend on this one...

lgsmith commented 3 years ago

What is missing at this point? I think I've addressed each change requested, though I may not have to @justinrporter 's satisfaction.

justinrporter commented 3 years ago

No not at all! From my perspective this is good to go!

The only thing is the tests. Were you able to run them locally? Unfortunately our test bot is broken at the moment; I was try to just do this for you and run the tests on my personal laptop but the new pip backtracking behavior is preventing me from installing your fork and running the tests :/ Sorry for the delay.

justinrporter commented 3 years ago

Got it working. LGTM

lgsmith commented 3 years ago

I actually found an issue with the test code where I hadn't correctly renamed the imports. Did it run OK for you?