dan2097 / jna-inchi

Wrapper to access InChI from Java
GNU Lesser General Public License v2.1
7 stars 6 forks source link

Failure to generate InChI for stereo molecule with JNA 5.13.0 and later #22

Closed lh598930 closed 4 months ago

lh598930 commented 4 months ago

We have a project that uses jna-inchi version 1.2. We recently added a new dependency to our project, which caused the indirect dependency on JNA to be updated from version 5.12.1 to 5.13.0. This change caused jni-inchi to break: some molecules for which an InChI was succesfully generated with JNA 5.12.1 now fail with the error message 'Atom ID is invalid'. The problem seems to affect molecules with stereochemistry. The same problem is exhibited with the latest version of JNA, 5.14.0.

We've found that we can work around the problem by replacing the dummy vertices with the actual indices in the DoubleBond case statement of the JnaInchi.addStereos(...) method, but we can't see how this fix works because we don't have access to the C code of jna-inchi's native methods. If you think it might take a while for you to investigate this problem, we'd appeciate it if you would make the code of the native methods available to us so that we can investigate the problem ourselves. Then, if we're able to find a fix, we'll make a pull request.

The following code fragment illustrates the problem:

InchiInput input = new InchiInput();

InchiAtom atom0 = new InchiAtom("F");
input.addAtom(atom0);

InchiAtom atom1 = new InchiAtom("C");
atom1.setImplicitHydrogen(1);
input.addAtom(atom1);

InchiAtom atom2 = new InchiAtom("C");
atom2.setImplicitHydrogen(1);
input.addAtom(atom2);

InchiAtom atom3 = new InchiAtom("Cl");
input.addAtom(atom3);

input.addBond(new InchiBond(atom0, atom1, InchiBondType.SINGLE));
input.addBond(new InchiBond(atom1, atom2, InchiBondType.DOUBLE));
input.addBond(new InchiBond(atom2, atom3, InchiBondType.SINGLE));

input.addStereo(InchiStereo.createDoubleBondStereo(atom0, atom1, atom2, atom3, InchiStereoParity.ODD));

InchiOutput output = JnaInchi.toInchi(input);
System.out.println(output.getStatus() + "  " + output.getMessage());

For version 5.12.1, the output is SUCCESS

For version 5.13.0, the output is ERROR Atom ID is invalid

dan2097 commented 4 months ago

The native code itself is provided by the IUPAC. I think https://github.com/IUPAC-InChI/InChI_1_06 has a copy of the source code used in the InChI version that is bundled, while https://github.com/IUPAC-InChI/InChI contains the latest code. IUPAC are currently beta testing a v1.07 release, although I haven't quite got around to testing this (https://github.com/dan2097/jna-inchi/issues/21)

I'll also see whether I can reproduce this issue, I have not tried JNA versions newer than the one which this project is bundled with

lh598930 commented 4 months ago

Thank you for your quick response. I misunderstood the role of JNA: I was expecting a wrapper that converted from Java types to C types. I should have read the JNA page on GitHub, particularly the bit that says "JNA provides Java programs easy access to native shared libraries without writing anything but Java code". Sorry.

I think the problem is in the IxaFunctions.IXA_ATOMID_IMPLICIT_H constant: the value of -1 needs to be a long, -1L, in order for the correct variant of the Pointer.createConstant(…) method to be called. It looks like the problem was introduced by this commit to JNA .

I made the change to the value of IxaFunctions.IXA_ATOMID_IMPLICIT_H, compiled the IxaFunctions class and replaced the class file in jna-inchi-api-1.2.jar. With the modified JAR, the code fragment that appears in my previous message now generates an InChI successfully with JNA 5.13.0.

lh598930 commented 4 months ago

Out of curiosity, why do you need to replace vertices 2 & 3 with dummy values when calling IxaFunctions.IXA_MOL_CreateStereoRectangle(…) from JnaInchi.addStereos(…) ?

dan2097 commented 4 months ago

Out of curiosity, why do you need to replace vertices 2 & 3 with dummy values when calling IxaFunctions.IXA_MOL_CreateStereoRectangle(…) from JnaInchi.addStereos(…) ?

If you imagine a double bond:

1a         2a
  \      /
    1 = 2 
  /      \
1b         2b

The configuration can be fully defined by whether 1a and 2b are on the same side or not e.g. the configuration can be defined without consideration of atoms 1b and 2a.

The IXA API expects the bond (i.e. 1-2), and then all four vertices (1a, 1b, 2a, 2b). In my code (https://github.com/dan2097/jna-inchi/blob/c0c3e5441ca3dcea2fea9be7ad6aaa826f8025b5/jna-inchi-api/src/main/java/io/github/dan2097/jnainchi/JnaInchi.java#L323) vertices 1/2/3/4 correspond to 1a, 1, 2, 2b i.e. I do not actually have pointers to the other two atoms. It would strictly speaking be more correct if I determined what these were, but I know from the InChI code base that it's just going to ignore them anyway! https://github.com/IUPAC-InChI/InChI_1_06/blob/320b9d3973bdbbdc3c60e1c5af505f0f38203d45/INCHI-1-SRC/INCHI_API/libinchi/src/ixa/ixa_builder.c#L1149

Personally I consider this a slight mistake in the InChI IXA API. The InChI IXA API converts its input to InChI's internal representation.... which just stores four atoms i.e. the same as JNA-InChI's representation.

dan2097 commented 4 months ago

That seems an odd change in JNA; they also changed their test code from Pointer.createConstant(-1); to Pointer.createConstant(-1L); so maybe it's reasonable. -1L also works fine on older versions of JNA.

Do you want me to do a bug fix release?

lh598930 commented 4 months ago

Thank you for explaining the rationale for the dummy vertices in JnaInchi.addStereos(…) .

Do you want me to do a bug fix release?

A release would be helpful to us, but please make a release only if you can do so without much trouble. If you're expecting to make a release for InChI 1.07 before long, we can wait for that.

dan2097 commented 4 months ago

A release would be helpful to us, but please make a release only if you can do so without much trouble. If you're expecting to make a release for InChI 1.07 before long, we can wait for that.

It'll probably be a while before InChI 1.07 is finalized (and binaries will also need to be built for the platforms InChI isn't shipped with) so I think a release makes sense. v1.2.1 is now on Maven Central.

lh598930 commented 4 months ago

Thank you for making the release of version 1.2.1, which has saved us the trouble of deploying our modified JAR.