Skip to content

refactor: DetectMolecules TY - #2561

Merged
trisyoungs merged 14 commits into
dissolve2/detect-molecules-refactor-version-2from
dissolve2/detect-molecules-refactor-version-2-ty
Jul 28, 2026
Merged

refactor: DetectMolecules TY#2561
trisyoungs merged 14 commits into
dissolve2/detect-molecules-refactor-version-2from
dissolve2/detect-molecules-refactor-version-2-ty

Conversation

@trisyoungs

Copy link
Copy Markdown
Member

No description provided.

Comment thread src/nodes/detectMolecules.cpp Outdated
// Loop over fragments of this size
while (!fragments.empty())
{
printf("FRAGs now %li\n", fragments.size());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using

const auto nFragments = fragments.size()

after line 174, we could make this abit nicer?

Suggested change
printf("FRAGs now %li\n", fragments.size());
printf("Found %li fragments\n", fragments.size());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Of course, nFragments - fragments.size() is the running number of fragments found within this current size class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, perhaps a better log would be

message("Found {} fragments of size {} \n", nFragments - fragments.size(), _);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, it's just a debug line I left in - I'll remove it now.

*/

// Copy atom and bond information from one structure to another
Structure &DetectMoleculesNode::copyStructureAtomsAndBonds(Structure &target, const std::vector<int> fragmentAtomIndices) const

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a small thing, but did copy...( not approach what is going on here as well as duplicate...( ?
Personally I prefer "copy", just feels more idiomatic.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think I called it duplicate... while I was reworking stuff to keep it separate, but either way happy to revert to copy.

@trisyoungs
trisyoungs merged commit 093c735 into dissolve2/detect-molecules-refactor-version-2 Jul 28, 2026
@trisyoungs
trisyoungs deleted the dissolve2/detect-molecules-refactor-version-2-ty branch July 28, 2026 08:35
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