search

CDK-R / cdkr

Integrating R and the CDK
https://cdk-r.github.io/cdkr/
42 stars 27 forks source link

Different behaviour for generate.formula in 3.4.9 #92

Closed sneumann closed 5 years ago

sneumann commented 5 years ago

Hi, I found the difference between the old and current rcdk behaviour, which caused https://github.com/MassBank/RMassBank/issues/199

rcdk_3.4.9.1 rcdklibs_2.2.1

generate.formula(mass=201.5031, window=0.006045093, elements=list(c("H", 0, 14), c("C", 0, 10), c("Cl", 0, 1), c("N", 0, 2)))
org.openscience.cdk.formula.MassToFormulaTool ERROR: Proposed mass is out of the range: 201.5031
list()

rcdk_3.4.7.1 rcdklibs_2.0

generate.formula(mass=201.5031, window=0.006045093, elements=list(c("H", 0, 14), c("C", 0, 10), c("Cl", 0, 1), c("N", 0, 2)))
Error in .jcall(mfSet, "I", "size") : 
  RcallMethod: invalid object parameter

Thing is, that RMassBank was trying to catch the error, and did not expect an empty list (see https://github.com/MassBank/RMassBank/issues/199#issuecomment-442193412). So, design question would be: do we want rcdk to return an empty formula list, or to throw an error ? Yours, Steffen

rajarshi commented 5 years ago

This is bad error handling on the rcdk side, since if the Java error was properly handled, you'd still get an empty list. In this case, the error has gone away because of updates on the CDK side (I think).

In any case, I'd vote for returning an empty list and do better handling of Java exceptions internally.

sneumann commented 5 years ago

Fully agreed, current behaviour is OK then. Yours, Steffen