Maygen #811

johnmay · 2022-02-01T09:15:35Z

Rebased - maygen PR.

… new Atom() directly.

… we can put this in the test/ root

…t for now we only need this function.

… instead

…r our IAtomContainer's.

…nning on a background thread.

javadev · 2022-02-02T14:01:41Z

tool/structgen/src/main/java/org/openscience/cdk/structgen/maygen/Maygen.java

@@ -2594,21 +2590,8 @@ public void structureGenerator(String localFormula) {
        List<int[]> newDegrees = distributeHydrogens();

        if (multiThread) {
-            try {
-                new ForkJoinPool(size)


size is important, it can't be removed.

Please explain, I asked @MehmetAzizYirik. My understanding this code is not doing anything since the parellel stream will not use the forjoin pool

Okay see here it really does work that way but is quite opaque/brittle - https://stackoverflow.com/questions/30802463/how-many-threads-are-spawned-in-parallelstream-in-java-8. "It should be noted that the Stream APIs use of ForkJoinPool is an implementation detail. Thus, both solutions work with Oracle’s current implementation but are not guaranteed to work everywhere."

Why size and not -

ForkJoinPool pool = new ForkJoinPool(Runtime.getRuntime().availableProcessors());

And that is the default anyways for ForkJoinPool in ParallelStream which is cleaner IMO.

@johnmay we had that conversation with Valentyn about the size parameter, but I believe this needs to be changed like you explained we can use that other option you recommended.

Also, this parallelization actually needs to be enhanced but further tasks not for now. Thus, we dont need to keep it with the ForkJoinPool but can change it later.

Please explain, I asked @MehmetAzizYirik. My understanding this code is not doing anything since the parellel stream will not use the forjoin pool

It is related for the bug MehmetAzizYirik/MAYGEN#48

In some configuration default value is 79. We need to control it not use default.

javadev · 2022-02-02T14:05:25Z

tool/structgen/src/test/java/org/openscience/cdk/structgen/maygen/MaygenCLI.java

+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+public class MaygenCLI {


The class may be removed. It is useless in the test folder.

javadev · 2022-02-02T14:06:03Z

tool/structgen/src/test/java/org/openscience/cdk/structgen/maygen/BoundaryConditionsTest.java

+
+    @Test
+    public void detectAllenes() {
+        assertFalse(BoundaryConditions.detectAllenes(new int[][] {}, new String[] {}));


Unit tests may be improved.

javadev · 2022-02-02T14:07:00Z

tool/structgen/pom.xml

+        <dependency>
+            <groupId>commons-cli</groupId>
+            <artifactId>commons-cli</artifactId>
+            <version>1.5.0</version>


We don't need this dependency.

javadev · 2022-02-02T14:08:57Z

tool/structgen/src/test/java/org/openscience/cdk/structgen/maygen/MaygenExpectedCountTest.java

+
+    @Test
+    public void testExpectedCountMultithreaded() throws CDKException, IOException, CloneNotSupportedException {
+        Maygen maygen = new Maygen(SilentChemObjectBuilder.getInstance());


Why we need to define SilentChemObjectBuilder.getInstance() in constructor?

So the structgen module is independent of implementation. Same reason it is provided to SmilesParser. Silent is faster than default which sends out notifications

It may be set parameter for class and default is SilentChemObjectBuilder.getInstance(). It is easier to create object with constructor without arguments.

The module does not and should not depend on data/silent. It’s just the way CDK is - I would not write it this way if we started again but “this is the way”

javadev · 2022-02-02T14:13:21Z

tool/structgen/src/main/java/org/openscience/cdk/structgen/maygen/Maygen.java

+     *  this is only used for formula validating and normalizing a parser would
+     *  be better. */
+    private static String replaceEach(String str, String[] from, String[] to) {
+        if (from.length != to.length)


It will be better to use other implementation with better error handling.

It would be better to not use this function at all. Using string munging to validate a formula is not nice. Adding in a third party library for a simple function - no

johnmay · 2022-02-04T14:42:04Z

@MehmetAzizYirik Why is the D₂ graph not supported in SMILES?

        if (writeSDF || printSDF) {
            IAtomContainer ac = buildContainer4SDF(mat);
            if (coordinates) new StructureDiagramGenerator().generateCoordinates(ac);
            sdfOut.write(ac);
        }
        if (writeSMILES || printSMILES) {
            smilesOut.write("Formula is not supported" + "\n");
        }

javadev · 2022-02-04T14:51:41Z

@MehmetAzizYirik Why is the D2 graph not supported in SMILES?

        if (writeSDF || printSDF) {
            IAtomContainer ac = buildContainer4SDF(mat);
            if (coordinates) new StructureDiagramGenerator().generateCoordinates(ac);
            sdfOut.write(ac);
        }
        if (writeSMILES || printSMILES) {
            smilesOut.write("Formula is not supported" + "\n");
        }

It caused an error as I remember.

johnmay · 2022-02-04T14:59:25Z

I am reading this correct that the way the fuzzy formulae are handled is by enumerating non-fuzzy formula? Hence why there seems to be some vestigial stats displayed to TSV.

MehmetAzizYirik · 2022-02-04T15:00:53Z

@MehmetAzizYirik Why is the D2 graph not supported in SMILES?

        if (writeSDF || printSDF) {
            IAtomContainer ac = buildContainer4SDF(mat);
            if (coordinates) new StructureDiagramGenerator().generateCoordinates(ac);
            sdfOut.write(ac);
        }
        if (writeSMILES || printSMILES) {
            smilesOut.write("Formula is not supported" + "\n");
        }

@johnmay As far as I remember, one of our colleagues recommended not to print SMILES for D2 graphs but I do not remember now the chemical reason she explained about the SMILES of D2 graphs. I am not sure whether there was an error or not.

MehmetAzizYirik · 2022-02-04T15:02:59Z

I am reading this correct that the way the fuzzy formulae are handled is by enumerating non-fuzzy formula? Hence why there seems to be some vestigial stats displayed to TSV.

@johnmay Yes, we needed the fuzzy formulae for another project, then I coded it like generating al possible standard formulae for given fuzzy formula. Can you please explain the thing about the TSV ? I did not get it.

johnmay · 2022-02-04T15:15:10Z

Just in the process of refactoring the IO - to me it looks like the TSVOUTPUT mode was probably just for the manuscript. I've left it as it is for now.

Is a D2 graph just one with all atoms degree 2 right? So in SMILES

C1CC1
C1CCC1
C1CCCC1
C1CCCCC1
C1CCCCCC1

but of course you could have other elements.

MehmetAzizYirik · 2022-02-04T15:28:19Z

Just in the process of refactoring the IO - to me it looks like the TSVOUTPUT mode was probably just for the manuscript. I've left it as it is for now.

Is a D2 graph just one with all atoms degree 2 right? So in SMILES

C1CC1 C1CCC1 C1CCCC1 C1CCCCC1 C1CCCCCC1

but of course you could have other elements.

@johnmay Just to be sure, I went through the code. Yes the SMILES could be given as string array as what we do in OnSm matrices. But I remember now the comment: "it is just a long list of S or O, and chemists are not interested in these molecules". I left it as how it is when I submitted the paper but in the future later we can change it like what we do with OnSm matrices. Same string array.

Also TSV, I remember for benchmarking while writing the paper, Chris recommended it would be better to have TSV option and he added it. That is all about TSV.

johnmay · 2022-02-04T16:24:26Z

Just in the process of refactoring the IO - to me it looks like the TSVOUTPUT mode was probably just for the manuscript. I've left it as it is for now.
Is a D2 graph just one with all atoms degree 2 right? So in SMILES
C1CC1 C1CCC1 C1CCCC1 C1CCCCC1 C1CCCCCC1
but of course you could have other elements.

@johnmay Just to be sure, I went through the code. Yes the SMILES could be given as string array as what we do in OnSm matrices. But I remember now the comment: "it is just a long list of S or O, and chemists are not interested in these molecules". I left it as how it is when I submitted the paper but in the future later we can change it like what we do with OnSm matrices. Same string array.

Also TSV, I remember for benchmarking while writing the paper, Chris recommended it would be better to have TSV option and he added it. That is all about TSV.

Okay… but then why is it useful in SDF?

MehmetAzizYirik · 2022-02-04T17:07:46Z

Just in the process of refactoring the IO - to me it looks like the TSVOUTPUT mode was probably just for the manuscript. I've left it as it is for now.
Is a D2 graph just one with all atoms degree 2 right? So in SMILES
C1CC1 C1CCC1 C1CCCC1 C1CCCCC1 C1CCCCCC1
but of course you could have other elements.

@johnmay Just to be sure, I went through the code. Yes the SMILES could be given as string array as what we do in OnSm matrices. But I remember now the comment: "it is just a long list of S or O, and chemists are not interested in these molecules". I left it as how it is when I submitted the paper but in the future later we can change it like what we do with OnSm matrices. Same string array.
Also TSV, I remember for benchmarking while writing the paper, Chris recommended it would be better to have TSV option and he added it. That is all about TSV.

Okay… but then why is it useful in SDF?

@johnmay There is no TSV usage in SDF, I checked the code but I think I did not understood what you meant. Can you please explain ?

johnmay · 2022-02-04T17:47:52Z

That was unrelated to the TSV comment sorry. What I'm trying to understand is

if (onlyDegree2) {
  if (oxygen == 0 || sulfur == 0) {
      degree2graph(); // when is this called, why does it only output to SDF and not SMILES?
  } else {
      distributeSulfurOxygen(normalizedLocalFormula);
  }
  displayStatistic(startTime, normalizedLocalFormula);
}

I presume this is only for custom valences since those are the only valence=2 default elements (well S should really be 2,4,6 but anyways). So my thoughts are something like this:
``
-f C(val=2)5 -smi -setElements


but that doesn't give me anything for neither SDF or SMILES.

johnmay · 2022-02-05T11:59:21Z

Remaining code smells would need a lot of work (i.e. wrapping up methods with larger number of params in a state object). One I can't work out is where the duplicated block is: https://sonarcloud.io/project/issues?id=cdk_cdk&pullRequest=811&resolved=false&rules=common-java%3ADuplicatedBlocks&types=CODE_SMELL

javadev · 2022-02-05T15:49:39Z

It looks good except CLI class in test package and commons-cli dependency. They may be removed.

johnmay · 2022-02-05T16:03:49Z

That’s fine for now - intention is these will go back into the maygen project repo (preferred) or we can add an assembly descriptor. TBD

MehmetAzizYirik · 2022-02-05T20:02:58Z

That’s fine for now - intention is these will go back into the maygen project repo (preferred) or we can add an assembly descriptor. TBD

@johnmay we can just do whatever you prefer like you explained above.

I am working on the test changes now and will do the pull request to the maygen branch.

And about parallelization, yes it needs changes like we talked but for now it can stay as how it is, I guess.

MehmetAzizYirik · 2022-02-05T20:06:15Z

Okay @egonw this is now more or less good to merge, I am going to have a look through SonarCloud and maybe fix any obvious smells - logging for example.

Not having to specify fuzzy formula and generally improved parsing and more decoupling would be good (main class is still 4000+ lines) would be good but I think it's time to get this merged. Oh also to fix the issue with ForkJoin I would propose making the multithreaded option an integer - e.g. on the command line "-j 8" or "-t 8".

@MehmetAzizYirik I can add a maven assembly descriptor to the POM so we can build the command line but I think a better option is once this is in a release you can rebase the MAYGEN repo to just contain the command line parts?

@johnmay yes john, as you recommended, I will do the changes after the release.

javadev · 2022-02-05T21:03:14Z

These changes ruins our logic btw.

We need to support smiles and sdf in the same time, not just sdf or smi.

It will be better to restore old logic.

MehmetAzizYirik · 2022-02-05T21:08:25Z

These changes ruins our logic btw.

We need to support smiles and sdf in the same time, not just sdf or smi.

It will be better to restore old logic.

@javadev if @johnmay thinks it is better not to return the both at the same time, then we keep it as how he changed the code.

MehmetAzizYirik · 2022-02-05T21:20:18Z

..... final thing to do is fix the tests writing data to disk.

@johnmay I checked now the structgen package and run the mvn test but no data was written to disk I checked. Did I misunderstand what you meant ?

johnmay · 2022-02-05T21:48:59Z

@MehmetAzizYirik no I did it all in the end. If you need SDF and SMILES you just write a new consumer that does both. I didn’t do this because on the command line tool it looked like smiles output would only activate if sdf wasn’t specified.

i’m very worn down with this PR now and still not 100% happy but it’s good enough now.

javadev · 2022-02-05T21:52:44Z

the command line tool it looked like smiles output would only activate if sdf wasn’t specified.

Support both formats at the same time is important.

johnmay · 2022-02-05T21:57:04Z

You can literally convert one in to the other so no it is really NOT important and a stupid design. But yea it’s still possible to do as I explained.

johnmay · 2022-02-05T21:57:37Z

I’m turning off notifications on this now - @egonw will take over.

MehmetAzizYirik · 2022-02-05T21:59:52Z

I’m turning off notifications on this now - @egonw will take over.

Thanks a lot for this detailed and helpful PR.

javadev · 2022-02-05T22:03:44Z

You can literally convert one in to the other

Sdf also supports coordinates.

I think the console application will behave unexpectedly in case you will use both keys sdf and smiles.

MehmetAzizYirik · 2022-02-05T22:48:14Z

You can literally convert one in to the other

Sdf also supports coordinates.

I think the console application will behave unexpectedly in case you will use both keys sdf and smiles.

@javadev as I wrote above, if John changed it, then done.

I did all my changes before the integration pull request, and after that, I left the code to CDK developers. It is a great pleasure for me if they do any changes on it and no need of any other changes after them.

javadev · 2022-02-06T06:41:37Z

I added support for the sdf and smi generator #818

MehmetAzizYirik · 2022-02-06T08:52:30Z

I added support for the sdf and smi generator #818

@javadev I as the main developer of MAYGEN , I dont want any other changes from someone else while CDK developers are working on it. And I prefer to keep it as how John changed! As it was your task in MAYGEN to clean code or format, if there is a need, we will let you know but besides that please do not resist to the changes of CDK developers, especially you are not in the position to criticize cheminformatics related work. I left the code to them after my changes and it is a pleasure if they enhance the code.

Added support for the smiles and sdf generator.

javadev · 2022-02-06T09:04:05Z

Hi @MehmetAzizYirik don't worry. These changes were mentioned by @johnmay in comments.

Changes have been approved btw.

sonarcloud · 2022-02-06T09:12:45Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
22 Code Smells

0.0% Coverage
0.5% Duplication

egonw · 2022-02-06T09:23:46Z

All, thank you very much for all your hard work, peer review, and positiveness. Very much appreciated!

MehmetAzizYirik · 2022-02-06T09:29:38Z

All, thank you very much for all your hard work, peer review, and positiveness. Very much appreciated!

@egonw Thanks to John and you for the guidance, this PR and all the changes you did. So happy that MAYGEN is integrated to CDK as planned in my PhD.

MehmetAzizYirik and others added 14 commits January 31, 2022 12:58

Integrating MAYGEN project to structgen package

c1d87ed

Added the JavaDoc to the source and test classes.

6bbfe59

Added dependencies, formatted sources.

be4d86e

Fix up some dependencies lost in the rebase. We should not be calling…

63b4b42

… new Atom() directly.

Split out the command line parsing from the main entry point. For now…

6b75b18

… we can put this in the test/ root

Pass in the IChemObjectBuilder

be00615

Fix copyright header

706b8f0

Remove commons-lang3, a specicalled MF range parse would be better bu…

158a34d

…t for now we only need this function.

Avoid reflective calls.

48da99f

removeHydrogens creates a copy of the molecule, use suppressHydrogens…

37e1457

… instead

Replace seperate output modes with a single "emit" which will consume…

a0dfd2b

…r our IAtomContainer's.

Remove redundant ForkJoinPool, submitting and then waiting is just ru…

01706c9

…nning on a background thread.

May SonarCloud happier by not using a for-loop here?

2c4b026

Parameterized tests

a9d8dd3

javadev reviewed Feb 2, 2022

View reviewed changes

johnmay added 3 commits February 5, 2022 11:35

Document usage of the class.

dfa4755

SonarCloud suggestion, move these tests into the Parameterized ones.

1d2b09d

Additional SonarCloud suggestions.

69ccdb7

This was referenced Feb 5, 2022

Integrating MAYGEN project to structgen package #801

Closed

Integrating MAYGEN project to structgen package #767

Closed

Added support for the smiles and sdf generator.

601f02f

Merge pull request #818 from jscrdev/maygen

eeefebd

Added support for the smiles and sdf generator.

egonw merged commit f9ecd05 into master Feb 6, 2022

johnmay deleted the maygen branch February 9, 2022 15:03

Maygen #811

Maygen #811

Conversation

johnmay commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnmay Feb 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javadev Feb 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javadev Feb 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnmay commented Feb 4, 2022

javadev commented Feb 4, 2022

johnmay commented Feb 4, 2022

MehmetAzizYirik commented Feb 4, 2022

MehmetAzizYirik commented Feb 4, 2022

johnmay commented Feb 4, 2022

MehmetAzizYirik commented Feb 4, 2022

johnmay commented Feb 4, 2022

MehmetAzizYirik commented Feb 4, 2022

johnmay commented Feb 4, 2022 • edited

johnmay commented Feb 5, 2022

javadev commented Feb 5, 2022

johnmay commented Feb 5, 2022

MehmetAzizYirik commented Feb 5, 2022

MehmetAzizYirik commented Feb 5, 2022

javadev commented Feb 5, 2022

MehmetAzizYirik commented Feb 5, 2022

MehmetAzizYirik commented Feb 5, 2022

johnmay commented Feb 5, 2022

javadev commented Feb 5, 2022

johnmay commented Feb 5, 2022

johnmay commented Feb 5, 2022

MehmetAzizYirik commented Feb 5, 2022

javadev commented Feb 5, 2022

MehmetAzizYirik commented Feb 5, 2022

javadev commented Feb 6, 2022

MehmetAzizYirik commented Feb 6, 2022 • edited

javadev commented Feb 6, 2022 • edited

sonarcloud bot commented Feb 6, 2022

egonw commented Feb 6, 2022

MehmetAzizYirik commented Feb 6, 2022

johnmay Feb 2, 2022 •

edited

javadev Feb 2, 2022 •

edited

javadev Feb 2, 2022 •

edited

johnmay commented Feb 4, 2022 •

edited

MehmetAzizYirik commented Feb 6, 2022 •

edited

javadev commented Feb 6, 2022 •

edited