galaxycomputationalchemistry / galaxy-tools-compchem

:mega: Galaxy Tools for Computational Chemistry
Apache License 2.0
14 stars 16 forks source link

A tool to analyze FEP calculations added #45

Closed tsenapathi closed 4 years ago

simonbray commented 4 years ago

This looks good @tsenapathi. I just proposed some small changes. :)

However I had problems with testing the tool. The provided test-data works fine, but when I submit my own data (that I generated with the alchemical_run tool, just a tarfile with 3 xvg files inside) I get an error like this:

Plotting the free energy breakdown figure...
Plotting the TI figure...
Traceback (most recent call last):
  File "/home/simon/miniconda3/envs/__alchemical-analysis@1.0.2/bin/alchemical_analysis", line 10, in <module>
    sys.exit(main())
  File "/home/simon/miniconda3/envs/__alchemical-analysis@1.0.2/lib/python2.7/site-packages/alchemical_analysis/alchemical_analysis.py", line 1285, in main
    plotdFvsLambda()
  File "/home/simon/miniconda3/envs/__alchemical-analysis@1.0.2/lib/python2.7/site-packages/alchemical_analysis/alchemical_analysis.py", line 1068, in plotdFvsLambda
    plotTI()
  File "/home/simon/miniconda3/envs/__alchemical-analysis@1.0.2/lib/python2.7/site-packages/alchemical_analysis/alchemical_analysis.py", line 1029, in plotTI
    xt = [i if (i in getInd()) else '' for i in range(K)]
TypeError: argument of type 'NoneType' is not iterable

Do you have any ideas?

tsenapathi commented 4 years ago

How long you have run the simulation? should be at least 100 ps.

simonbray commented 4 years ago

Ah, that makes sense, my simulations are only 1 ps. I'll try and generate some longer ones.

tsenapathi commented 4 years ago

@simonbray @bgruening @chrisbarnettster any more comments or suggestions?

simonbray commented 4 years ago

Not from me

simonbray commented 4 years ago

Actually, wait, a couple more things occurred to me:

tsenapathi commented 4 years ago

@simonbray smaller data set added Can you point me to an example tool.yml file? and why we need to add that?

tsenapathi commented 4 years ago

@bgruening @chrisbarnettster @simonbray The condition for skip lambda removed and tried to add a condition based on whether this parameter is given or not. Did not work!! any idea how to do this?

tsenapathi commented 4 years ago

@bgruening @simonbray Good to merge?

simonbray commented 4 years ago

There are some things that could still be added:

tsenapathi commented 4 years ago

@simonbray I have a .shed.yml see https://github.com/tsenapathi/galaxy-tools-compchem/blob/master/tools/free_energy/.shed.yml. @bgruening agreed to go with .tar files for now, and in future, I will create a new data type such as xvg.tar.

I will add a validator now

simonbray commented 4 years ago

Okay. But please don't make an xvg.tar datatype. A collection is the correct solution longterm.

tsenapathi commented 4 years ago

@bgruening @simonbray can I push the updated gmx_run tool to this PR

simonbray commented 4 years ago

@tsenapathi, yes sounds good

tsenapathi commented 4 years ago

@simonbray The alchemical analysis test passing on my local installation but keep failing here. Any idea why?

simonbray commented 4 years ago

I'll try testing locally

simonbray commented 4 years ago

@tsenapathi, my best guess is that my change to the travis config is causing this. Tests are also passing locally for me. I'm reverting it here: https://github.com/galaxycomputationalchemistry/galaxy-tools-compchem/pull/48

tsenapathi commented 4 years ago

Cool Thanks!!

tsenapathi commented 4 years ago

@simonbray @chrisbarnettster @bgruening any idea why this is happening?

tsenapathi commented 4 years ago

@simonbray great! So it needed pymbar.

simonbray commented 4 years ago

Nice, it worked! :D

@tsenapathi, the culprit was the new version of pymbar that was released on conda three days ago. It has some incompatibility with alchemical_analysis. As conda always installs the newest version automatically it's been failing since then.

simonbray commented 4 years ago

@bgruening, we can merge afaic. Any other comments on your side?