AlkylStructureFragmenter Implementation #10

Mila1004 · 2023-07-07T12:41:04Z

First steps for a complete alkyl structure fragmentation.
Currently only contains fragmentation logic for cyclic structures and non-cyclic side-chains.

…rst algorithm setting

…detection;

…rst algorithm setting

…detection;

…MORTAR into dev-Max-alkylfragmentation

…Fragmenter;

… duplicate rings;

sonarcloud · 2024-04-26T20:52:44Z

Quality Gate failed

Failed conditions
7.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Mila1004 · 2024-04-26T20:57:00Z

...va/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenterTest.java

+        for (IAtomContainer tmpAC : anACSet.atomContainers()) {
+            AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(tmpAC);
+            tmpAdder.addImplicitHydrogens(tmpAC);
+            Kekulization.kekulize(tmpAC);


It seems the kekulization of the resulting fragments after successful fragmentation leads to an exception originating from unset implicit hydrogens, even though they should be added by the CDKHydrogenAdder before starting kekulization. The kekulization of the expected fragments does not throw any exceptions as the aromaticity is already set by directly reading from a file.

Apparently, you have two fragments of SMILES code "*C(*)*" in your resulting fragments set. I have looked at the detected atom types and their implicit hydrogen counts. Somehow, I don't know how, they are treated differently here. In the first structure, every R-/pseudo-atom has a hydrogen count of 0, and in the second structure, it is "null", causing the issue.But the CDKHydrogenAdder should ignore all of the pseudo atoms (see here: https://github.com/cdk/cdk/blob/01d7ab0ea4f5f0f590b42cbb2c45e32e2cf41066/base/valencycheck/src/main/java/org/openscience/cdk/tools/CDKHydrogenAdder.java#L98). An interesting bug or feature: if you use the atom-specific method, it does not check for pseudo-atoms and treats them "right", i.e. assigns an implicit hydrogen count of 0 to them. Then, your test does not throw an exception but the assertions fails.

The kekulization of the expected fragments does not throw any exceptions as the aromaticity is already set by directly reading from a file.

Partly true, kekulization of the expected fragments is successful because explicit bond orders can be assigned correctly. We can discuss the theoretical background on Friday.

I have looked at the detected atom types and their implicit hydrogen counts. Somehow, I don't know how, they are treated differently here. In the first structure, every R-/pseudo-atom has a hydrogen count of 0, and in the second structure, it is "null", causing the issue.

I saw that too and was very confused by it.

An interesting bug or feature: if you use the atom-specific method, it does not check for pseudo-atoms and treats them "right", i.e. assigns an implicit hydrogen count of 0 to them.

I will test this too in the coming days.

We can discuss the theoretical background on Friday.

Gladly, I would like to know the underlying theory.

…ion;

…detection;

…y carbons;

…n test class;

…est class;

sonarcloud · 2024-09-05T12:29:13Z

Quality Gate failed

Failed conditions
5.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

JonasSchaub · 2024-09-16T12:12:47Z

@Mila1004, when you have time, please merge the current production branch into your branch and adjust your code to the changes made there in the meantime.

JonasSchaub

Incomplete review, will continue tomorrow.

JonasSchaub · 2024-09-16T12:04:45Z

src/main/java/de/unijena/cheminf/mortar/model/fragmentation/FragmentationService.java

+    /**
+     * Conjugated Pi System Fragmenter
+     */
+    private IMoleculeFragmenter ConjPiSysF;


The two new fragmenter variables should go to the final class constants like the others.

JonasSchaub · 2024-09-16T12:06:54Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+TODO: (01|08|24):   -implement spiro carbon detection + ring integrity setting
+                    -overhaul test class (with new functionalities)
+                    -large scale testing of fragmentation (regarding correct fragmentation and performance)
+ */


Finish to dos or move them to GH issues or discussion

JonasSchaub · 2024-09-16T12:09:34Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+    //</editor-fold>
+    //
+    //<editor-fold desc="Public Properties Get">
+    @Override


The code fold is not working for me (some others as well). It helps to add a line break between the fold declaration and the "'at'Override".

JonasSchaub · 2024-09-16T12:18:38Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+    /**
+     * Default boolean value for determining which single ring detection method should be used.
+     */
+    public static final boolean ALTERNATIVE_SINGLE_RING_DETECTION_SETTING_DEFAULT = false;


We need to discuss the setting naming

JonasSchaub · 2024-09-16T12:25:52Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+    /**
+     * A property that has a constant boolean value defining if rings should be dissected further.
+     */
+    private final SimpleBooleanProperty keepRingsSetting;


Are all these settings/properties used now? Why the "constant value" statement?

JonasSchaub · 2024-09-16T14:40:39Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+                    }
+                }
+            } catch (Exception anException) {
+                AlkylStructureFragmenter.this.logger.log(Level.WARNING,


no need to log

And the logger is static, so there should be no "this" inbetween.

And I will stop commenting "no need to log" now.

JonasSchaub · 2024-09-16T14:44:12Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+        } else {
+            //<editor-fold desc="RingSearch (isolated)">
+            //for spiro detection: set array property for each atom belonging to x rings
+            //--> if more than one ring --> check for connected atoms to determine if spiro config


So, the setting switches between detecting isolated rings or detecting all rings again using MCB? Does this actually make a difference?

JonasSchaub · 2024-09-16T14:52:00Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+        IAtomContainer tmpIsolatedMultiBondsContainer = new AtomContainer();
+        IAtomContainer tmpTertQuatCarbonContainer = new AtomContainer();
+        IAtomContainer tmpSpiroCarbonContainer = new AtomContainer();
+        IAtomContainer tmpSpiroResidueContainer = new AtomContainer();


spiro residue container is unused

JonasSchaub · 2024-09-16T14:58:57Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+            if (!((boolean) tmpAtom.getProperty(AlkylStructureFragmenter.INTERNAL_ASF_RING_MARKER_KEY)
+                    || (boolean) tmpAtom.getProperty(AlkylStructureFragmenter.INTERNAL_ASF_CONJ_PI_MARKER_KEY))) {
+                if (tmpAtom.getProperty(AlkylStructureFragmenter.INTERNAL_ASF_TERTIARY_CARBON_PROPERTY_KEY)) {
+                    tmpRingFragmentationContainer.addAtom(tmpAtom);


Why use the ring fragmentation container here when you have a dedicated container for tertiary and quaternary carbons?

JonasSchaub · 2024-09-16T15:01:33Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+                if (!this.keepRingsSetting.get()) {
+                    ArrayList<Integer> tmpList = tmpAtom.getProperty(AlkylStructureFragmenter.INTERNAL_ASF_RING_ATOM_LIST_KEY);
+                    if (tmpList.size() > 1) {
+                        tmpSpiroCarbonContainer.addAtom(tmpAtom);


Ok, if the atom is part of multiple isolated rings, it must be a spiro carbon. But what if the mcb ring detection is used?

JonasSchaub · 2024-09-17T11:22:59Z

@Mila1004 where did you get the example molecules in the MOL and SD files from? There should be a source/reference given somewhere.

JonasSchaub

In the fragmenter, it's mostly small things but the test class needs to be improved a lot.

JonasSchaub · 2024-09-17T08:18:44Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+                    }
+                    ArrayList<Integer> tmpEndList = tmpEndAtom.getProperty(AlkylStructureFragmenter.INTERNAL_ASF_RING_ATOM_LIST_KEY);
+                    if (tmpEndList.size() > 1) {
+                        tmpIsEndSpiro = true;


JonasSchaub · 2024-09-17T08:23:12Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+            //check maxchainlength
+            switch (tmpMaxChainLengthInteger) {
+                default -> {
+                    //restrictions > 1


(style) default case should be the last one in a switch

JonasSchaub · 2024-09-17T08:24:34Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+                default -> {
+                    //restrictions > 1
+                    for (IAtomContainer tmpAtomContainer : tmpChainACSet.atomContainers()) {
+                        tmpDissectedChainACSet.add(this.separateDisconnectedStructures(this.dissectLinearChain(tmpAtomContainer,


separating method calls into multiple lines makes it easier to debug.

JonasSchaub · 2024-09-17T08:25:43Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+                case 0 -> {
+                    //if int maxChainLength gives 0 throw IllegalArgumentException
+                    //not happy with how this works, preferably a gui warning would be nice
+                    AlkylStructureFragmenter.this.logger.log(Level.WARNING, "Illegal restriction argument", new IllegalArgumentException());


This should be defined in the constructor where you initialise the setting properties. Check the other fragmenters.

Also, the default value is 0, right? Defaults should not raise exceptions but should be generally well-suited valaues!

JonasSchaub · 2024-09-17T08:45:57Z

...n/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenter.java

+        IAtomContainer tmpReturnAC = new AtomContainer();
+        //starts at 1 for usability, see aLength: 1on1 translation of input to counter
+        int tmpCounter = 1;
+        Iterator<IBond> tmpBondIterator = anAC.bonds().iterator();


Can you really rely on the iterator starting at one end of the chain, traversing along it, and ending at the other end?

JonasSchaub · 2024-09-17T12:04:04Z

...va/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenterTest.java

+        } catch (FileNotFoundException e) {
+            throw new RuntimeException(e);
+        } catch (URISyntaxException e) {
+            throw new RuntimeException(e);


again, no need to catch. I will not comment this down below.

JonasSchaub · 2024-09-17T12:05:05Z

...va/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenterTest.java

+
+    //<editor-fold desc="@Test Custom Methods">
+    @Test
+    public void detectRingsWithMCBTest() {


This test does n your functionality but the cycle finder.

JonasSchaub · 2024-09-17T12:05:36Z

...va/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenterTest.java

+
+    }
+    @Test
+    public void detectRingsWithRingSearchTest() {


Again, tests of the used CDK functionalities do not belong here.

JonasSchaub · 2024-09-17T12:07:34Z

...va/de/unijena/cheminf/mortar/model/fragmentation/algorithm/AlkylStructureFragmenterTest.java

+        return tmpACSet;
+    }
+    //</editor-fold>
+


As stated in the comment above from the last review, this test class needs more step-by-step examples with actual molecules (check the SugarRemovalUtility test).

JonasSchaub · 2024-09-17T12:11:39Z

...e/unijena/cheminf/mortar/model/fragmentation/algorithm/ConjugatedPiSystemFragmenterTest.java

+            tmpFragmentList = tmpFragmenter.fragmentMolecule(tmpOriginalMolecule);
+            SmilesGenerator tmpGenerator = new SmilesGenerator(SmiFlavor.Canonical);
+            for (IAtomContainer tmpFragment : tmpFragmentList) {
+                System.out.println(tmpGenerator.create(tmpFragment) + " "


Assert instead of print.

…g structures;

sonarcloud · 2024-11-07T16:00:23Z

Quality Gate failed

Failed conditions
7.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Mila1004 and others added 30 commits April 20, 2023 15:54

added new algorithm AlkylStructureFragmenter.java; exemplary added fi…

0d207da

…rst algorithm setting

added further algorithm settings without functionality

8011316

WIP, Does not Compile! started implementing filter for alkyl structures

e796701

added alkyl structure filter functionality in shouldBeFiltered;

89988ad

added carbohydrate filter; added longest chain detection; added ring …

5c283d3

…detection;

added different ring detection algorithm;

9555f0a

WIP! working on ring/chain detection

a3f5ab2

WIP! working on carbon chain detection

543f941

added new algorithm AlkylStructureFragmenter.java; exemplary added fi…

41b8474

…rst algorithm setting

added further algorithm settings without functionality

9326006

WIP, Does not Compile! started implementing filter for alkyl structures

6b71b46

added alkyl structure filter functionality in shouldBeFiltered;

c563270

added carbohydrate filter; added longest chain detection; added ring …

7139641

…detection;

added different ring detection algorithm;

c0ae663

WIP! working on ring/chain detection

1800a8e

WIP! working on carbon chain detection

ca4a5c8

Merge branch 'dev-Max-alkylfragmentation' of github.com:FelixBaensch/…

9c9d1ca

…MORTAR into dev-Max-alkylfragmentation

Depict structure image with fill to fit depending on atom count;

23cc8e5

WIP! working on chain detection and extraction

2e328bb

WIP! working on carbon chain detection

7c75ece

WIP! Does not compile!

cbc1c84

WIP! working on alkyl chain detection

e59e0f7

WIP! fixed alkyl detection bug;

b49017c

added detection of conjugated pi systems; fixed filter bug;

e06b346

WIP! working on ring system detection

0c83d87

WIP! working on chain detection and extraction

02d12d6

added ConjugatedPiSystemDetector implementation as ConjugatedPiSystem…

7eaa4d2

…Fragmenter;

WIP! working on ring detection and fragment return;

276e9f7

WIP! added hydrogen saturation; added ring system detection;

a5645d4

WIP! overhauled ring and ring system detection; working on removal of…

31a2788

… duplicate rings;

Mila1004 commented Apr 26, 2024

View reviewed changes

Mila1004 and others added 18 commits June 11, 2024 22:33

fixed issue with hydrogen saturation in ASF and its test class;

69fa1c0

added check for correct molecular formula/atom count after fragmentat…

26c6481

…ion;

added rule in molecular formula check for ignoring CDK pseudo atoms;

b06caac

added switch/setting for alternative single carbon handling;

74574d8

fixed bug in ASF test class imported molecule saturation;

7bca034

added tests for different ring detection methods;

4d4f007

WIP! spiro carbon detection and extraction;

f38737d

WIP! on isolated double-triple bond extraction; WIP! on spiro carbon …

bba7d15

…detection;

WIP! on isolated double and triple bond extraction;

cc96daa

added extraction of isolated double and triple bonds;

d04d734

WIP extraction of neighbor atoms of tertiary and quaternary carbons;

1ad9741

WIP extraction of neighbor atoms of tertiary and quaternary carbons;

cd9aa96

WIP! in general; added neighbor extraction for tertiary and quaternar…

b363891

…y carbons;

WIP! rework of general fragment extraction;

9064e97

reworked ring and conjugated pi system marking; WIP! on test class;

9bf6a3e

fixed falsely placed isolated rings in fused ring systems bug; WIP! o…

0e47cc6

…n test class;

added toggleable extraction of spiro configuration carbons; WIP! on t…

1e5c822

…est class;

reworked basic tests for AlkylStructureFragmenter;

4bcfa15

JonasSchaub requested changes Sep 16, 2024

View reviewed changes

JonasSchaub requested changes Sep 17, 2024

View reviewed changes

Mila1004 added 4 commits October 14, 2024 12:04

merge branch 'production' into dev-Max-alkylfragmentation;

498151e

minor adjustments and changes to fragmentation and settings;

480cd41

further adjustments regarding code quality;

0c089b5

WIP! fixed bug in extraction of double bonded carbon connected to rin…

1bfcc22

…g structures;

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AlkylStructureFragmenter Implementation #10

AlkylStructureFragmenter Implementation #10

Mila1004 commented Jul 7, 2023

sonarcloud bot commented Apr 26, 2024

Mila1004 Apr 26, 2024 •

edited

Loading

JonasSchaub Apr 29, 2024 •

edited

Loading

JonasSchaub Apr 29, 2024

Mila1004 Apr 29, 2024

sonarcloud bot commented Sep 5, 2024

JonasSchaub commented Sep 16, 2024

JonasSchaub left a comment

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub Sep 16, 2024

JonasSchaub commented Sep 17, 2024

JonasSchaub left a comment

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

JonasSchaub Sep 17, 2024

sonarcloud bot commented Nov 7, 2024

AlkylStructureFragmenter Implementation #10

Are you sure you want to change the base?

AlkylStructureFragmenter Implementation #10

Conversation

Mila1004 commented Jul 7, 2023

sonarcloud bot commented Apr 26, 2024

Quality Gate failed

Mila1004 Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

JonasSchaub Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 5, 2024

Quality Gate failed

JonasSchaub commented Sep 16, 2024

JonasSchaub left a 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

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

JonasSchaub commented Sep 17, 2024

JonasSchaub left a 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

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

sonarcloud bot commented Nov 7, 2024

Quality Gate failed

Mila1004 Apr 26, 2024 •

edited

Loading

JonasSchaub Apr 29, 2024 •

edited

Loading