Skip to content

Exhaustive fragmenter saturation fix and optimization#1

Closed
ToLeWeiss wants to merge 47 commits intomainfrom
exhaustive_fragmenter
Closed

Exhaustive fragmenter saturation fix and optimization#1
ToLeWeiss wants to merge 47 commits intomainfrom
exhaustive_fragmenter

Conversation

@ToLeWeiss
Copy link
Copy Markdown
Owner

@ToLeWeiss ToLeWeiss commented Feb 6, 2025

Fix for Saturation Issues in ExhaustiveFragmenter (#1119)

Aims to resolve the saturation-related issues in the ExhaustiveFragmenter, as described in issue #1119.

Fixes & Improvements

Ensures Correct Fragment Saturation:

  • The generated fragments are now explicitly copied, preventing unintended modifications.
  • Added the ability to set and control the saturation state of returned fragments.

Optimized Bond Splitting Logic:

  • Implemented an optimization based on the observation that a specific combination of bonds only needs to be split once.
  • This reduces redundant computations, improving overall efficiency.

Improvements

  • Fixes inconsistencies in fragment saturation.
  • Improves performance by avoiding unnecessary recomputations.
  • Enhances flexibility by allowing users to specify saturation preferences.

Testing & Verification

  • Verified that fragment saturation is correctly applied across different configurations.
  • Tested for performance improvements with large molecular structures.
  • Ensured no regressions in existing fragmentation behavior.

… saturated or unsaturated fragments, added a test and renamed some tests to prepare for the tests for unsaturated fragments
… algorithm changes to use a power set to reduce computations
…ve or too small because of truncation of BigInt to int
…se and therefore cant have more elements than 2^31 - 1
…ion and added more comprehensive comments for subset generation
Bringing this feature branch up to date.
@ToLeWeiss ToLeWeiss requested a review from JonasSchaub February 6, 2025 10:29
Copy link
Copy Markdown
Collaborator

@JonasSchaub JonasSchaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review part I, continuation later.

Copy link
Copy Markdown
Collaborator

@JonasSchaub JonasSchaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second and final part of first review.

@JonasSchaub
Copy link
Copy Markdown
Collaborator

@JonasSchaub
Copy link
Copy Markdown
Collaborator

@ToLeWeiss another thing to think about: is there any chance to have a substructure/fragment that is there multiple times in the input structure also appear multiple times in the fragments, despite the deduplication using SMILES?
E.g. if I split C1CCCCC1C2CCCCC2, I would like to have two cyclohexanes in the output, not just one.

@ToLeWeiss ToLeWeiss requested a review from JonasSchaub June 4, 2025 12:47
@JonasSchaub
Copy link
Copy Markdown
Collaborator

@ToLeWeiss I finished my review. Please have a look at the comments and talk to me if anything is unclear.

@ToLeWeiss ToLeWeiss requested a review from JonasSchaub August 20, 2025 15:48
Copy link
Copy Markdown
Collaborator

@JonasSchaub JonasSchaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of ExhaustiveFragmenter class.

@JonasSchaub
Copy link
Copy Markdown
Collaborator

@ToLeWeiss the code is becoming really nice!
The main remaining points are the preservation of stereochemistry, introducing another constructor, and clearing up some confusion about the inclusive max tree depth (see my comments). Everything else was mostly comments.

@ToLeWeiss ToLeWeiss requested a review from JonasSchaub August 21, 2025 17:12
…added a setting for copying setereo information + tests
@ToLeWeiss ToLeWeiss closed this Oct 23, 2025
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

Successfully merging this pull request may close these issues.

2 participants