Integration of CDK ExhaustiveFragmenter #133

JonasSchaub · 2024-10-07T08:39:45Z

No description provided.

…fragmentation utility of the CDK, as a fragmentation option

… of the minimal fragment size of the exhaustive fragmenter

…f the exhaustive fragmentation and added the respective test class

…ue to automatic saturation

JonasSchaub · 2024-10-23T12:00:18Z

@ToLeWeiss , I fear performance will be a big issue here. I just started a fragmentation with 1000 natural products and it is still running. We need to discuss this, test it more, and maybe document it somewhere.

Final result of my run:

Oct 23, 2024 1:45:35 PM de.unijena.cheminf.mortar.controller.MainViewController lambda$importMoleculeFile$25
INFO: Successfully imported 1000 molecules from file: Picked_COCONUT_subset_1000_mols.sdf; 0 molecules could not be parsed into the internal data model (SMILES code generation failed). See above how many molecules could not be read from the input file at all or produced exceptions while preprocessing.
Oct 23, 2024 1:45:41 PM de.unijena.cheminf.mortar.controller.MainViewController startFragmentation
INFO: Start of method startFragmentation
Oct 23, 2024 1:45:41 PM de.unijena.cheminf.mortar.model.fragmentation.FragmentationService startFragmentation
INFO: Fragmentation "Exhaustive Fragmenter" (Exhaustive Fragmenter) starting. Current memory consumption: 168 MB
Oct 23, 2024 2:23:38 PM de.unijena.cheminf.mortar.model.fragmentation.FragmentationService startFragmentation
INFO: Fragmentation "Exhaustive Fragmenter" (Exhaustive Fragmenter) of 1,000 molecules complete. It took 2,277,137 ms. Current memory consumption: 2,496 MB
Oct 23, 2024 2:23:38 PM de.unijena.cheminf.mortar.model.fragmentation.FragmentationService startSingleFragmentation
INFO: Number of different fragments extracted: 169,490
Oct 23, 2024 2:23:39 PM de.unijena.cheminf.mortar.controller.MainViewController lambda$startFragmentation$44
INFO: End of method startFragmentation after 2277.6163057

And afterwards, it aborted with an out of memory error...
But as you can see, 169,490 fragments were generated from 1,000 natural products in 38 minutes (4 parallel threads and while I was also working on other things on the same laptop).

JonasSchaub

Looks good overall, just some minor things in the comments.

JonasSchaub · 2024-10-23T11:42:57Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+        if (!this.shouldBePreprocessed(aMolecule)) {
+            return aMolecule.clone();
+        }
+        return aMolecule.clone();


The if statement is unnecessary. I guess you wanted to leave room between the if statement and the return statement for actual preprocessing should you need to add it later? If yes, please add a comment, at least. Or simply remove the if.

JonasSchaub · 2024-10-23T12:03:48Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+ * <a href="https://cdk.github.io/cdk/latest/docs/api/org/openscience/cdk/fragment/ExhaustiveFragmenter.html">
+ *     exhaustive fragmentation
+ * </a>
+ * from the CDK, available for MORTAR.


unnecessary comma

JonasSchaub · 2024-10-23T12:04:58Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+    /**
+     * The name of the algorithm used for fragmentation.
+     */
+    private static final String ALGORITHM_NAME = "Exhaustive Fragmenter";


These two should be public.

JonasSchaub · 2024-10-23T12:08:00Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+        this.settingNameTooltipTextMap = new HashMap<>(tmpNumberOfSettings,
+                BasicDefinitions.DEFAULT_HASH_COLLECTION_LOAD_FACTOR);
+        this.settingNameDisplayNameMap = new HashMap<>(tmpNumberOfSettings,
+                BasicDefinitions.DEFAULT_HASH_COLLECTION_LOAD_FACTOR);


Please use CollectionUtil.calculateInitialHashCollectionCapacity() or HashMap.newHashMap().

I see now that you only have one setting but are initialising the maps with a capacity of 2, ok. But for the future, please use the ways I indicated.

JonasSchaub · 2024-10-23T12:19:47Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+        this.settingNameDisplayNameMap = new HashMap<>(tmpNumberOfSettings,
+                BasicDefinitions.DEFAULT_HASH_COLLECTION_LOAD_FACTOR);
+        this.cdkEFInstance = new ExhaustiveFragmenter();
+        this.minimumFragmentSize = new SimpleIntegerProperty(this,


I would put -Setting at the end of this variable or -Preference. Otherwise, one might expect an integer here.

JonasSchaub · 2024-10-23T12:40:46Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+        }
+        //</editor-fold>
+        IAtomContainer tmpMoleculeClone = aMolecule.clone();
+        List<IAtomContainer> tmpFragments = new ArrayList<>(0);


Think of a more suiting default initial size, like maybe the number of bonds in the molecule times two?

JonasSchaub · 2024-10-23T12:41:25Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+        IAtomContainer tmpMoleculeClone = aMolecule.clone();
+        List<IAtomContainer> tmpFragments = new ArrayList<>(0);
+        try {
+            SmilesParser tmpSmilesParser = new SmilesParser(DefaultChemObjectBuilder.getInstance());


Please use SilentChemObjectBuilder

JonasSchaub · 2024-10-23T12:42:04Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+            SmilesParser tmpSmilesParser = new SmilesParser(DefaultChemObjectBuilder.getInstance());
+            this.cdkEFInstance.generateFragments(tmpMoleculeClone);
+            // there is also an option to extract atom containers directly with getFragmentsAsContainers but this oversaturates
+            // fragments described in this issue https://github.com/cdk/cdk/issues/1119.


Good that you put this here but please flag it with TODO:

JonasSchaub · 2024-10-23T12:46:47Z

...ava/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenterTest.java

+        IAtomContainer tmpOriginalMolecule;
+        List<IAtomContainer> tmpFragmentList;
+        CDKExhaustiveFragmenter tmpFragmenter = new CDKExhaustiveFragmenter();
+        tmpFragmenter.setFragmentSaturationSetting(IMoleculeFragmenter.FragmentSaturationOption.HYDROGEN_SATURATION);


This does not make much sense, given that you deactivated that feature.

JonasSchaub · 2024-10-23T12:53:58Z

...in/java/de/unijena/cheminf/mortar/model/fragmentation/algorithm/CDKExhaustiveFragmenter.java

+
+    @Override
+    public FragmentSaturationOption getFragmentSaturationSetting() {
+        //TODO: there is currently no possibility to implement saturation settings for the exhaustive fragmenter.


Can you describe this in more detail here like you did below?
And the way to do it here would be to throw an UnsupportedOperationException in these three methods. But please test whether this impacts running MORTAR. Since you do not add the property to the list of settings, we should be fine.

JonasSchaub · 2024-10-23T12:57:45Z

When the new fragmenter is added, we also need to describe it in the tutorial: https://docs.google.com/document/d/1dbpSZfdmOYaQqdYTDZj0NfKuy9TMp6T4SF6lCBB7bFo/edit?usp=sharing
@ToLeWeiss could you suggest some edits there? I need to redo some of the pictures, so that they are consistent.

…xhaustive fragmenter class

JonasSchaub · 2024-11-07T09:53:06Z

@ToLeWeiss ready for re-review?

sonarcloud · 2024-11-07T10:16:27Z

Quality Gate failed

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

See analysis details on SonarCloud

ToLeWeiss · 2024-11-07T10:17:29Z

Now it is ready for a review.

Initialization of new fragmenter class

34cb816

JonasSchaub added the enhancement New feature or request label Oct 7, 2024

JonasSchaub changed the title ~~Initialization of new fragmenter class~~ Integration of CDK ExhaustiveFragmenter Oct 7, 2024

ToLeWeiss added 7 commits October 12, 2024 20:44

work in progress: first rudimentary implementation of the exhaustive …

8e5d817

…fragmentation utility of the CDK, as a fragmentation option

work in progress: added hydrogen saturation and corrected the setting…

0f03124

… of the minimal fragment size of the exhaustive fragmenter

work in progress: functional implementation, except the saturation, o…

80e2381

…f the exhaustive fragmentation and added the respective test class

work in progress: fixed formatting error and modified fragmentation d…

03492bf

…ue to automatic saturation

work in progress: removed the saturation option

37e45d7

finalized implementation

e40d160

small refactor for consistency with other fragmenters

e9161a8

ToLeWeiss marked this pull request as ready for review October 22, 2024 15:18

JonasSchaub commented Oct 23, 2024

View reviewed changes

ToLeWeiss added 2 commits October 28, 2024 05:04

minor fixes

34e8885

applied requested fixes and added some further documentation to the e…

75f31a2

…xhaustive fragmenter class

correction for the performance and added further information

8d5c796

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration of CDK ExhaustiveFragmenter #133

Integration of CDK ExhaustiveFragmenter #133

JonasSchaub commented Oct 7, 2024

JonasSchaub commented Oct 23, 2024 •

edited

Loading

JonasSchaub left a comment

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub Oct 23, 2024

JonasSchaub commented Oct 23, 2024

JonasSchaub commented Nov 7, 2024

sonarcloud bot commented Nov 7, 2024

ToLeWeiss commented Nov 7, 2024

Integration of CDK ExhaustiveFragmenter #133

Are you sure you want to change the base?

Integration of CDK ExhaustiveFragmenter #133

Conversation

JonasSchaub commented Oct 7, 2024

JonasSchaub commented Oct 23, 2024 • edited Loading

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

JonasSchaub commented Oct 23, 2024

JonasSchaub commented Nov 7, 2024

sonarcloud bot commented Nov 7, 2024

Quality Gate failed

ToLeWeiss commented Nov 7, 2024

JonasSchaub commented Oct 23, 2024 •

edited

Loading