search

MassBank / RMassBank

Playground for experiments on the official http://bioconductor.org/packages/devel/bioc/html/RMassBank.html
Other
12 stars 15 forks source link

OpenBabel and other issues #280

Closed trachtok closed 3 years ago

trachtok commented 3 years ago

Hi,

I would like to report few issues I came across when wrapping your great RMassBank package as a tool for Galaxy. Some of these issues, I presume, could be easily solved by changing the R code for RMassBank a bit so I am putting them here, although they are not actual errors in code but more ideas for improvement of the RMassBank code.

  1. OpenBabel issue 1, right now the RMassBank 3.0 version has hard-coded execution of babel, but the newest OpenBabel changed the name of the function to obabel, so it's not possible to use the newest version of OpenBabel. I am by no means and OpenBabel expert so I am not sure if there are some other significant differences between the OpenBabel versions, but if the RMassBank could take both babel and obabel it would be great.

  2. OpenBabel issue 2, RMassBank requires a path to the babel, and it's not possible to give it an empty argument so that it would just call babel in the current directory. It would be great if the RMassBank could take maybe an empty argument and in that case, just call OpenBabel in the directory where the tool is running, without any path specification.

  3. OpenBabel issue 3, if the path to OpenBabel specified in settings file exists BUT there is no babel executable in the path, there is an uncaught error on line 415 of createMassBank.R res <- system(cmd, intern=TRUE, input=smiles, ignore.stderr=TRUE) (and if you - like me - specified the wrong path to OpenBabel in the settings file, the error is not very informative).

  4. Last small issue is about required suffixes for input files, this caused me some trouble because Galaxy internally renames all uploaded input files and gives them suffix .dat, which is of course refused by RMassBank so I have to symlink all the input files. This can of course be done but it's rather messy and removing the requirement for correct suffix from RMassBank code would solve this (since suffix 'mzML' or 'csv' does not really ensure that the file is of that format anyway).

If you would be interested into looking at our wrapped RMassBank for Galaxy, you can find it here. Thank you for all your hard work on this great package!

Karolína

tsufz commented 3 years ago

Dear Karolína, Thanks a lot for your message. We will check the issues and try to release an update as soon as possible.

Best wishes, Tobias

tsufz commented 3 years ago

@trachtok could you text the version of your OpenBabel. ? The latest stable version is 2.4.1 from 2016 and thus your problems might be caused by a different issue.

Best, Tobias

trachtok commented 3 years ago

Hi @tsufz, thank you for looking into this! I am using OpenBabel 2.4.1 (installed via conda), but when I initially installed RMassBank (3.0.0 also via conda) and newest OpenBabel (3.1.1 on conda) there is no executable babel, there is only obabel. It seems that obabel is now preferred, see their documentation here. I did not know this when I was setting up conda env for RMassBank so I automatically went for the newest version of Openbabel. And when I specified the OpenBabel path in the settings file to the conda env directory, it of course did not find babel (as there was only obabel) and I get the error from issue 3, because although the path exist, there is no babel. So I rolled back to OpenBabel 2.4.1 and then it works just fine :).

tsufz commented 3 years ago

Hi @trachtok, many thanks again for the hint. OpenBabel is quite relaxed to update their homepage. I was always wondering why there no new version was released, and I was not aware that they moved to GitHub. We will take care to properly update our code and the documentation.

Have a nice weekend, Tobias

tsufz commented 3 years ago

New OpenBabel repository: https://github.com/openbabel/openbabel

sneumann commented 3 years ago

So we need 1) a unit test with something RMB would be doing with babel, and 2) then switch to obabel in the system calls. We don't need a fallback to non-o-babel, since obabel has been around for so long. Yours, Steffen

tsufz commented 3 years ago

Yes, it was already available in the older versions, but for any reason we did not catch the problem so far.

schymane commented 3 years ago

babel itself was only removed from the releases quite recently so it has probably only just become a problem. It should be quite a quick fix.

tsufz commented 3 years ago

Okay, I talk to @pstahlhofen later this day anyway.

tsufz commented 3 years ago

Closed with #285.