refactor: Detect molecules#2528
Open
RobBuchananCompPhys wants to merge 9 commits into
Open
Conversation
RobBuchananCompPhys
force-pushed
the
dissolve2/refactor-detect-molecules
branch
from
353a3e6 to
cd27f8f
Compare
RobBuchananCompPhys
changed the base branch from
dissolve2/detect-molecules
to
develop2
RobBuchananCompPhys
force-pushed
the
dissolve2/refactor-detect-molecules
branch
from
cd27f8f to
150dd4c
Compare
RobBuchananCompPhys
force-pushed
the
dissolve2/refactor-detect-molecules
branch
2 times, most recently
from
2b5a4f2 to
6e08831
Compare
RobBuchananCompPhys
force-pushed
the
dissolve2/refactor-detect-molecules
branch
from
a160f32 to
9303bb2
Compare
trisyoungs
reviewed
Jun 26, 2026
trisyoungs
left a comment
trisyoungs
left a comment
Member
There was a problem hiding this comment.
This is excellent and a whole lot cleaner than the old one, but I think there are some looping / workflow errors to address!
Member
There was a problem hiding this comment.
Does this still need a rebase? This was in #2530 ...
Comment on lines
+28
to
+29
| for (int i = 0; i < structure.nAtoms(); i++) | ||
| fragments.emplace_back(fragment(i)); |
Member
There was a problem hiding this comment.
Shouldn't this function start from the next un-selected atom? Need something like a std::set to track which atoms have been detected within instances (a bug that doesn't show up for the atomic NaCl or MgO cases!)
| detectedStructures_.clear(); | ||
|
|
||
| // Return all discovered molecular fragment index vectors | ||
| auto allFragmentIndices = findMolecularFragments(inputStructure_); |
Member
There was a problem hiding this comment.
If you're pre-detecting all bound fragments here (which is a good idea)....
|
|
||
| std::set<const StructureAtom *> atomMask; | ||
|
|
||
| for (int i = 0; i < inputStructure_.nAtoms(); i++) |
Member
There was a problem hiding this comment.
...why is this a loop over structure atoms and not fragments?
Comment on lines
+94
to
+138
| // Set up the return value and bind its contents | ||
| NETADefinition bestNETA; | ||
| std::vector<StructureAtom *> rootAtoms; | ||
|
|
||
| // Maintain a set of atoms matched by any NETA description we generate | ||
| std::set<StructureAtom *> alreadyMatched; | ||
|
|
||
| // Skip this atom? | ||
| if (alreadyMatched.find(fragmentAtom) != alreadyMatched.end()) | ||
| continue; | ||
|
|
||
| // Create a NETA definition with this atom as the root | ||
| NETADefinition neta; | ||
| neta.create(static_cast<AtomBase *>(fragmentAtom), std::nullopt, | ||
| Flags<NETADefinition::NETACreationFlags>(NETADefinition::NETACreationFlags::ExplicitHydrogens, | ||
| NETADefinition::NETACreationFlags::IncludeRootElement)); | ||
|
|
||
| // Apply this match over the whole species | ||
| std::vector<StructureAtom *> currentRootAtoms; | ||
| for (auto fragAtomIndex : fragmentIndices) | ||
| { | ||
| const auto fragmentAtom = inputStructure_.atom(fragAtomIndex); | ||
| if (neta.matches(fragmentAtom)) | ||
| { | ||
| currentRootAtoms.push_back(fragmentAtom); | ||
| alreadyMatched.insert(fragmentAtom); | ||
| } | ||
| } | ||
|
|
||
| // Is this a better description? | ||
| auto better = false; | ||
| if (rootAtoms.empty() || currentRootAtoms.size() < rootAtoms.size()) | ||
| better = true; | ||
| else if (currentRootAtoms.size() == rootAtoms.size()) | ||
| { | ||
| // Replace the current match if there are more bonds on the current atom. | ||
| if (fragmentAtom->nBonds() > rootAtoms.front()->nBonds()) | ||
| better = true; | ||
| } | ||
|
|
||
| if (better) | ||
| { | ||
| bestNETA = neta; | ||
| rootAtoms = currentRootAtoms; | ||
| } |
Member
There was a problem hiding this comment.
This whole NETA creation part needs to run on each atom within a fragment, reason being to find the best, most concrete way of describing uniquely the bonding / atoms within it. Then it can be applied to each fragment (of the same size) in turn to see if the NETA matches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.