Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate NonStandard InChI #253

Closed
johnmay opened this issue Dec 10, 2016 · 10 comments
Closed

Generate NonStandard InChI #253

johnmay opened this issue Dec 10, 2016 · 10 comments

Comments

@johnmay
Copy link
Member

johnmay commented Dec 10, 2016

Via Nina:

Using InChIGenerator with non-standard options generates standard InChI (starting with 1S/ ) with 1.5.14. The same code generated non-standard InChI in previous versions (1.5.13 and before)

List<INCHI_OPTION> options = new ArrayList<INCHI_OPTION>();
options.add(INCHI_OPTION.FixedH);
options.add(INCHI_OPTION.SAbs);
options.add(INCHI_OPTION.SAsXYZ);
options.add(INCHI_OPTION.SPXYZ);
options.add(INCHI_OPTION.FixSp3Bug);
options.add(INCHI_OPTION.AuxNone);
InChIGeneratorFactory factory = InChIGeneratorFactory.getInstance();
InChIGenerator gen1 = factory.getInChIGenerator(mol1, options);
@johnmay
Copy link
Member Author

johnmay commented Dec 13, 2016

@vedina have you been able to confirm if this is a dependency problem?

@vedina
Copy link
Contributor

vedina commented Dec 13, 2016

it certainly affects cdk 1.5.14 and not cdk 1.5.13.

had not checked if the underlying inchi library is different - is it?

@johnmay
Copy link
Member Author

johnmay commented Dec 13, 2016

No it's not - but there are lots of test cases that generate non-standard InChIs thats didn't fail. Can you check on 2.0-SNAPSHOT.

John

@johnmay
Copy link
Member Author

johnmay commented Jan 6, 2017

@vedina will close 1 day please let me know if you want to keep open

@vedina
Copy link
Contributor

vedina commented Jan 6, 2017

will check during the weekend, there was a problem when upgrading to 1.5.14

vedina added a commit to ideaconsult/examples-cdk that referenced this issue Jan 7, 2017
@vedina
Copy link
Contributor

vedina commented Jan 7, 2017

@johnmay here is the test

https://github.com/ideaconsult/examples-cdk/blob/master/maven-single-module/src/test/java/net/idea/examples/cdk/maven_single_module/InchiTest.java

Works with cdk <=1.5.13, fails with 1.5.14 . There is maven profile for CDK versions .

@vedina
Copy link
Contributor

vedina commented Jan 7, 2017

@johnmay it's weird, the tests (including the one in CDK) fail only on Windows machines (both JDK 7 and 8). In fact other tests in the cdk/storage/inchi folder are also failing on Windows.

Any hints what might have changed since 1.5.13 will be very useful!

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.openscience.cdk.graph.invariant.InChINumbersToolsTest
Tests run: 14, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.963 sec <<< FAILURE! - in org.openscience.cdk.graph.invariant.InChINumbersToolsTest
fixedH(org.openscience.cdk.graph.invariant.InChINumbersToolsTest)  Time elapsed: 0.105 sec  <<< FAILURE!
java.lang.AssertionError: 
Expected: is "AuxInfo=1/1/N:6,7,5,8,2,4,9,3,1/E:(1,2)(3,4)(6,7)(8,9)/F:7,6,8,5,2,9,4,1,3/rA:9NCNCCCCCC/rB:s1;d2;s3;d4;s5;d6;s7;s1s4d8;/rC:;;;;;;;;;"
     but: was "AuxInfo=1/1/N:6,7,5,8,2,4,9,3,1/E:(1,2)(3,4)(6,7)(8,9)/rA:9NCNCCCCCC/rB:s1;d2;s3;d4;s5;d6;s7;s1s4d8;/rC:;;;;;;;;;"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at org.openscience.cdk.graph.invariant.InChINumbersToolsTest.fixedH(InChINumbersToolsTest.java)

Running org.openscience.cdk.inchi.InChIGeneratorFactoryTest
Tests run: 12, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.256 sec <<< FAILURE! - in org.openscience.cdk.inchi.InChIGeneratorFactoryTest
testGetInChIGenerator_IAtomContainer_List(org.openscience.cdk.inchi.InChIGeneratorFactoryTest)  Time elapsed: 0 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<InChI=1[]/ClH/h1H> but was:<InChI=1[S]/ClH/h1H>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.openscience.cdk.inchi.InChIGeneratorFactoryTest.testGetInChIGenerator_IAtomContainer_List(InChIGeneratorFactoryTest.java)

testGetInChIGenerator_IAtomContainer_String(org.openscience.cdk.inchi.InChIGeneratorFactoryTest)  Time elapsed: 0 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<InChI=1[]/ClH/h1H> but was:<InChI=1[S]/ClH/h1H>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.openscience.cdk.inchi.InChIGeneratorFactoryTest.testGetInChIGenerator_IAtomContainer_String(InChIGeneratorFactoryTest.java)

@johnmay
Copy link
Member Author

johnmay commented Jan 7, 2017

Ahhhh that makes more sense. Putting this together with something I saw on the InChI mailing list. InChI in it's infinite wisdom has different syntax depending on the OS. Windows uses '/' and everything else uses '-'.

Windows:
inchi-1 /STDIO
Everything Else:
inchi-1 -STDIO

When we modified the handling to allow the 15T/KET etc I added output as '-KET -15T' etc. I really thought the '/' was not needed at the API level... but apparently this is the case.

I'll get this fixed ASAP, hopefully by tomorrow.

Here (https://github.com/cdk/cdk/blob/master/storage/inchi/src/main/java/org/openscience/cdk/inchi/JniInChIInputAdapter.java#L79) the '-' needs to be replaced with a constant based on OS (like line separation).

@vedina
Copy link
Contributor

vedina commented Jan 8, 2017

I'see ....

@egonw egonw closed this as completed in #260 Jan 8, 2017
vedina added a commit to ideaconsult/examples-cdk that referenced this issue Mar 27, 2018
1.5.14 has issue with inchi on windows cdk/cdk#253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants