From 87c9b06dadcec0b3cd8b371bfaef2a3ef5b642be Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Mon, 22 Jun 2026 15:02:24 +0100 Subject: [PATCH 1/9] refactor detect molecules process --- src/classes/fragment.h | 25 ++ src/classes/structure.cpp | 9 + src/classes/structure.h | 5 + src/nodes/detectMolecules.cpp | 296 ++++++++++++++++++ src/nodes/detectMolecules.h | 65 ++++ src/nodes/insert.cpp | 91 +++++- src/nodes/insert.h | 24 +- src/nodes/parameter.h | 1 + src/nodes/registry.cpp | 2 + src/nodes/test.cpp | 37 +++ src/nodes/test.h | 5 + tests/nodes/cif.cpp | 546 ++++++++++++++++++++++------------ tests/nodes/parameters.cpp | 66 ++++ 13 files changed, 976 insertions(+), 196 deletions(-) create mode 100644 src/nodes/detectMolecules.cpp create mode 100644 src/nodes/detectMolecules.h diff --git a/src/classes/fragment.h b/src/classes/fragment.h index c623cca757..0d309532d2 100644 --- a/src/classes/fragment.h +++ b/src/classes/fragment.h @@ -28,6 +28,24 @@ template class Fragment getIndicesRecursive(atoms, indices, j->index(), exclusions); } } + static void getIndicesRecursive(const std::vector> &atoms, std::vector &indices, int index, + const std::vector &exclusions) + { + // Loop over bonds on indexed atom + indices.emplace_back(index); + const auto i = atoms.at(index).get(); + for (const auto *bond : i->bonds()) + { + // Is this either of the excluded bonds? + if (std::ranges::find(exclusions, bond) != exclusions.end()) + continue; + + // Get the partner atom in the bond and select it (if it is not selected already) + auto j = bond->partner(i); + if (std::find(indices.begin(), indices.end(), j->index()) == indices.end()) + getIndicesRecursive(atoms, indices, j->index(), exclusions); + } + } public: // Return the fragment (vector of indices) containing the specified atom @@ -38,4 +56,11 @@ template class Fragment getIndicesRecursive(atoms, indices, startIndex, exclusions); return indices; } + static std::vector get(const std::vector> &atoms, int startIndex, + const std::vector &exclusions = {}) + { + std::vector indices; + getIndicesRecursive(atoms, indices, startIndex, exclusions); + return indices; + } }; diff --git a/src/classes/structure.cpp b/src/classes/structure.cpp index 2c20f0d8a2..261afcbd10 100644 --- a/src/classes/structure.cpp +++ b/src/classes/structure.cpp @@ -14,15 +14,20 @@ Structure &Structure::operator=(const Structure &source) { clear(); + // Copy atoms for (auto &atom : source.atoms_) { auto &i = atoms_.emplace_back(std::make_unique()); i->copy(*atom); } + // Copy bonds for (auto &bond : source.bonds_) addBond(bond->i()->index(), bond->j()->index()); + // Copy instances + instances_ = source.instances_; + // Copy source box createBox(source.box_.axisLengths(), source.box_.axisAngles(), source.box_.type() == Box::BoxType::None); @@ -119,6 +124,10 @@ const StructureAtom *Structure::atom(int i) const { return atoms_[i].get(); } const std::vector> &Structure::atoms() const { return atoms_; } std::vector> &Structure::atoms() { return atoms_; } +// Return molecular species coordinates +const std::vector> &Structure::instances() const { return instances_; } +std::vector> &Structure::instances() { return instances_; } + /* * Connectivity */ diff --git a/src/classes/structure.h b/src/classes/structure.h index 0dd41d3c43..e90b52067a 100644 --- a/src/classes/structure.h +++ b/src/classes/structure.h @@ -52,6 +52,8 @@ class Structure : public Serialisable private: // Atoms in the structure std::vector> atoms_; + // Positional instances of the root structure + std::vector> instances_; private: // Renumber atoms so they are sequential in the vector @@ -74,6 +76,9 @@ class Structure : public Serialisable // Return atoms const std::vector> &atoms() const; std::vector> &atoms(); + // Return positional instances of the root structure + const std::vector> &instances() const; + std::vector> &instances(); /* * Connectivity diff --git a/src/nodes/detectMolecules.cpp b/src/nodes/detectMolecules.cpp new file mode 100644 index 0000000000..8f41dc1b76 --- /dev/null +++ b/src/nodes/detectMolecules.cpp @@ -0,0 +1,296 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +// Copyright (c) 2026 Team Dissolve and contributors + +#include "nodes/detectMolecules.h" +#include "classes/empiricalFormula.h" +#include "classes/fragment.h" +#include "classes/molecule.h" +#include "classes/species.h" +#include +#include + +DetectMoleculesNode::DetectMoleculesNode(Graph *parentGraph) : Node(parentGraph) +{ + // Inputs + addInput("Structure", "Input structure", inputStructure_); +} + +/* + * Definition + */ + +std::string_view DetectMoleculesNode::type() const { return "DetectMolecules"; } + +std::string_view DetectMoleculesNode::summary() const { return "Detect molecular species within a structure"; } + +/* + * Processing + */ + +// Run main processing +NodeConstants::ProcessResult DetectMoleculesNode::process() +{ + detectedStructures_.clear(); + + // Unfold structure + inputStructure_.unFold(); + + // Return all discovered molecular fragment index vectors + auto fragmentMap = findMolecularFragments(inputStructure_); + + // Define lambda for capturing any molecular instances we detect + auto appendInstances = [&](std::vector> &instances, Structure &detectedMolecularStructure, + const std::vector &fragment) -> bool + { + if (!instances.empty()) + { + detectedStructures_.emplace_back(copyStructureAtomsAndBonds(inputStructure_, detectedMolecularStructure, fragment)) + .instances() = instances; + + return true; + } + return false; + }; + + // Try selecting within the species from the first atom - if this captures all atoms we have a bound framework... + if (fragmentMap.contains(inputStructure_.nAtoms())) + return error( + "Can't create molecular definitions since this unit cell appears to be a continuous framework/network. Consider " + "adjusting the bonding options in order to generate molecular fragments.\n"); + + std::set atomMask; + + for (const auto &[size, fragments] : fragmentMap) + for (const auto &fragment : fragments) + { + // Create a provisional structure for the detected fragment + Structure detectedStructure; + detectedStructure.createBox(inputStructure_.box().axes()); + std::vector> instances; + + // Get fragment atoms + auto fragmentAtoms = getFragmentAtoms(inputStructure_, fragment); + + // Remove fragments that are larger than 50 % of the structure + if (size * 2 > inputStructure_.nAtoms()) + { + addInstance(instances.emplace_back(), fragmentAtoms); + + // Mask these fragment atoms + for (const auto &unmasked : fragmentAtoms) + atomMask.insert(unmasked); + + appendInstances(instances, detectedStructure, fragment); + + break; + } + + for (const auto &fragmentAtom : fragmentAtoms) + { + if (atomMask.contains(fragmentAtom)) + continue; + + /* + * Best NETA definition + */ + + // Set up the return value and bind its contents + NETADefinition bestNETA; + std::vector rootAtoms; + + // Maintain a set of atoms matched by any NETA description we generate + std::set 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(fragmentAtom), std::nullopt, + Flags(NETADefinition::NETACreationFlags::ExplicitHydrogens, + NETADefinition::NETACreationFlags::IncludeRootElement)); + + // Apply this match over the whole species + std::vector currentRootAtoms; + for (auto fragmentAtomIndex : fragment) + { + const auto fragmentAtom = inputStructure_.atom(fragmentAtomIndex); + 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; + } + + /* + * Get instances + */ + + auto fragmentSizeGroupAtoms = getFragmentAtoms(inputStructure_, fragments); + + // Iterate over all structural atoms, matching their unit cell atoms by NETA + std::vector> matchedUnitCellAtomSets; + for (const auto &fragmentAtom : fragmentSizeGroupAtoms) + { + if (atomMask.contains(fragmentAtom)) + continue; + + auto matchedPath = neta.matchedPath(fragmentAtom).set(); + if (!matchedPath.empty()) + { + auto set = matchedUnitCellAtomSets.emplace_back(matchedPath); + + // Mask the current matched fragment atom + for (const auto &matchedAtom : set) + atomMask.insert(static_cast(matchedAtom)); + } + } + + // Loop over matched unit cell atoms, retrieving instances + for (const auto &matchedUnitCellAtoms : matchedUnitCellAtomSets) + { + if (matchedUnitCellAtoms.empty()) + continue; + + addInstance(instances.emplace_back(), matchedUnitCellAtoms); + } + } + + appendInstances(instances, detectedStructure, fragment); + } + + message("Detected {} distinct fragment structures:\n\n", detectedStructures_.size()); + message(" ID N Species Formula\n"); + auto count = 1; + for (const auto &structure : detectedStructures_) + message(" {:3d} {:4d} {}\n", count++, structure.instances().size(), + EmpiricalFormula::formula(structure.atoms(), [](const auto &i) { return i->Z(); })); + message(""); + + /* + * Dynamic outputs + */ + + // Register dynamic outputs + for (int i = 0; i < detectedStructures_.size(); i++) + { + auto val = detectedStructures_[i]; + auto paramName = std::string("DetectedMolecule" + std::format("-{}", i)); + + // Check if output already exists - do not add if it does + if (outputs_.find(paramName) != outputs_.end()) + continue; + + addOutput(paramName, "Detected molecular structure", detectedStructures_[i]); + } + + return NodeConstants::ProcessResult::Success; +} + +/* + * Helpers + */ + +// Copy atom and bond information from one structure to another +Structure &DetectMoleculesNode::copyStructureAtomsAndBonds(const Structure &source, Structure &target, + const std::vector fragmentAtomIndices) +{ + // Copy fragment atoms, forming a map of the original indices to the new atom in the structure + std::map originalIndexMap; + for (auto fragAtomIndex : fragmentAtomIndices) + { + const auto fragmentAtom = source.atom(fragAtomIndex); + originalIndexMap[fragAtomIndex] = target.addAtom(fragmentAtom->Z(), fragmentAtom->r(), fragmentAtom->q()); + std::cout << std::format("New atom added to structure: {} {}\n", fragAtomIndex, Elements::symbol(fragmentAtom->Z())); + } + + // Copy bond information - since our fragment is by definition a bound fragment, we copy all bonds on each atom + for (auto fragAtomIndex : fragmentAtomIndices) + { + const auto fragmentAtom = source.atom(fragAtomIndex); + for (auto bond : fragmentAtom->bonds()) + { + // Add a bond between the new atoms in the detected structure (as long as it doesn't already exist) + if (!target.hasBond(originalIndexMap[bond->i()->index()], originalIndexMap[bond->j()->index()])) + target.addBond(originalIndexMap[bond->i()->index()], originalIndexMap[bond->j()->index()]); + } + } + + return target; +} + +// Add fragment molecular instance +void DetectMoleculesNode::addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms) +{ + std::visit( + [&](const auto &atoms) + { + for (const AtomBase *atom : atoms) + targetInstance.push_back(atom->r()); + }, + instanceFragmentAtoms); +} + +// Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments +std::vector DetectMoleculesNode::getFragmentAtoms(const Structure &structure, + const std::vector &fragmentIndices) +{ + std::vector fragmentAtoms; + for (const auto &fragmentAtomIndex : fragmentIndices) + fragmentAtoms.push_back(structure.atom(int(fragmentAtomIndex))); + + return fragmentAtoms; +} + +// Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments +std::vector DetectMoleculesNode::getFragmentAtoms(const Structure &structure, + const FragmentVector &fragmentIndices) +{ + std::vector indices; + std::size_t newSize = 0; + for (const auto &v : fragmentIndices) + ++newSize; + indices.reserve(newSize); + for (const auto &v : fragmentIndices) + indices.insert(indices.end(), v.begin(), v.end()); + + return getFragmentAtoms(structure, indices); +} + +// Find all molecular fragments +std::map DetectMoleculesNode::findMolecularFragments(const Structure &structure) +{ + std::map map; + + auto fragment = [structure](int i) { return Fragment>::get(structure.atoms(), i); }; + + for (int i = 0; i < structure.nAtoms(); i++) + { + auto element = fragment(i); + const int size = element.size(); + if (!map.contains(size)) + map.emplace(size, FragmentVector{}); + auto &targetFragments = map[size]; + targetFragments.push_back(element); + } + + return map; +} \ No newline at end of file diff --git a/src/nodes/detectMolecules.h b/src/nodes/detectMolecules.h new file mode 100644 index 0000000000..095649c884 --- /dev/null +++ b/src/nodes/detectMolecules.h @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +// Copyright (c) 2026 Team Dissolve and contributors + +#pragma once + +#include "classes/localMolecule.h" +#include "classes/molecule.h" +#include "classes/species.h" +#include "classes/structure.h" +#include "data/elements.h" +#include "math/vector3.h" +#include "nodes/node.h" +#include +#include +#include + +class Species; + +// DetectMolecules Node +class DetectMoleculesNode : public Node +{ + using FragmentVector = std::vector>; + using AtomCollection = std::variant, std::vector>; + + public: + DetectMoleculesNode(Graph *parentGraph); + ~DetectMoleculesNode() override = default; + + public: + std::string_view type() const override; + std::string_view summary() const override; + + /* + * Definition + */ + private: + // Input structure + Structure inputStructure_; + // Output structures + std::vector detectedStructures_; + + /* + * Helpers + */ + private: + // Copy atom and bond information from one structure to another + static Structure ©StructureAtomsAndBonds(const Structure &source, Structure &target, + const std::vector fragmentIndices); + // Add fragment molecular instance + static void addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms); + // Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments + static std::vector getFragmentAtoms(const Structure &structure, + const std::vector &fragmentIndices); + static std::vector getFragmentAtoms(const Structure &structure, + const FragmentVector &fragmentIndices); + // Find all molecular fragments + static std::map findMolecularFragments(const Structure &structure); + + /* + * Processing + */ + protected: + // Run main processing + NodeConstants::ProcessResult process() override; +}; diff --git a/src/nodes/insert.cpp b/src/nodes/insert.cpp index 6460eacfb0..b3282e7580 100644 --- a/src/nodes/insert.cpp +++ b/src/nodes/insert.cpp @@ -8,20 +8,25 @@ #include "kernels/energy.h" #include "math/mathFunc.h" #include "nodes/dissolve.h" +#include +#include +#include InsertNode::InsertNode(Graph *parentGraph) : Node(parentGraph) { // Inputs addInput("Configuration", "Target configuration to insert into", configuration_); addOutput("Configuration", "Modified configuration", configuration_); - addInput("Species", "Species to add - all resulting molecules will have identical geometry", species_); - addInput("MoleculeSet", "MoleculeSet to use as the source", moleculeSet_); addInput("Population", "Population of the target to add", population_); addInput("Density", "Density at which to add the target", density_); + addInput("Species", "Source species or molecule set to add - all resulting molecules will have identical geometry", + speciesVariant_); + addInput("Instances", "", instances_); // Options addOption("DensityUnits", "Units of target density", densityUnits_); addOption("BoxAction", "Action to take on the Box geometry / volume on addition of the species", boxAction_); + addOption("InstantiationMethod", "Strategy for instantiation of species during insertion", instantiationMethod_); addOption("ScaleA", "Scale box length A when modifying volume", scaleA_); addOption("ScaleB", "Scale box length B when modifying volume", scaleB_); addOption("ScaleC", "Scale box length C when modifying volume", scaleC_); @@ -51,6 +56,18 @@ EnumOptions InsertNode::boxActionStyles() } EnumOptions getEnumOptions(InsertNode::BoxActionStyle) { return InsertNode::boxActionStyles(); } +// Return enum option info for InstantiationMethod +EnumOptions InsertNode::instantiationMethod() +{ + return EnumOptions("InstantiationMethod", + {{InsertNode::InstantiationMethod::Sample, "Sample"}, + {InsertNode::InstantiationMethod::InstantiateAll, "InstantiateAll"}}); +} +EnumOptions getEnumOptions(InsertNode::InstantiationMethod) +{ + return InsertNode::instantiationMethod(); +} + /* * Processing */ @@ -147,11 +164,17 @@ NodeConstants::ProcessResult InsertNode::process() { // Get target MoleculeSet MoleculeSet speciesMoleculeSet; - if (species_) - speciesMoleculeSet.addMolecule(species_); - const MoleculeSet &targetMoleculeSet = species_ ? speciesMoleculeSet : *moleculeSet_; + auto insertFromSpecies = speciesVariant_.isAlternative(std::type_index(typeid(const Species *))); + if (insertFromSpecies) + speciesMoleculeSet.addMolecule(std::get(speciesVariant_.data)); + const MoleculeSet &targetMoleculeSet = + insertFromSpecies ? speciesMoleculeSet : *std::get(speciesVariant_.data); + + // Bool flag - do we have instances for this species + auto hasInstances = !instances_.instances().empty(); - auto ipop = population_.asInteger(); + auto ipop = hasInstances && instantiationMethod_ == InstantiationMethod::InstantiateAll ? instances_.instances().size() + : population_.asInteger(); if (ipop <= 0) { warn("Population is zero so nothing will be added.\n"); @@ -175,6 +198,35 @@ NodeConstants::ProcessResult InsertNode::process() break; } + std::random_device rd; + std::mt19937 gen(rd()); + std::uniform_int_distribution<> distr(0, ipop - 1); + std::set alreadySampled; + + const auto sampleInstances = [&alreadySampled, &distr, &gen, ipop, this]() + { + const auto instances = this->instances_.instances(); + + int index = 0; + + // We've run out of new unsampled instances + if (alreadySampled.size() == instances.size()) + return -1; + + while (true) + { + auto sampledIndex = distr(gen); + if (alreadySampled.contains(sampledIndex)) + continue; + + index += sampledIndex; + alreadySampled.insert(index); + break; + } + + return index; + }; + Matrix3 transform; const auto &box = configuration_->box(); configuration_->atoms().reserve(configuration_->atoms().size() + nAnyAtoms); @@ -183,6 +235,33 @@ NodeConstants::ProcessResult InsertNode::process() // Add the Molecule auto mol = configuration_->copyMolecule(targetMoleculeSet.localMolecule(n)); + auto insertionComplete = false; + + // If we have instances, either instantiate all from current positions, or sample from them randomly and/or randomise + // position of Molecule over the whole box + if (hasInstances) + { + std::vector atomicCoords; + if (instantiationMethod_ == InstantiationMethod::InstantiateAll) + { + atomicCoords = instances_.instances()[n]; + insertionComplete = true; + } + else + { + auto instancesIndex = sampleInstances(); + if (instancesIndex < 0) + break; + } + + // Update molecular atomic coordinates + for (int i = 0; i < mol->nAtoms(); i++) + mol->atom(i)->setR(atomicCoords[i]); + + if (insertionComplete) + continue; + } + // Randomise position of Molecule over the whole box auto newCentre = box.getReal({DissolveMath::random(), DissolveMath::random(), DissolveMath::random()}); mol->setCentreOfGeometry(box, newCentre); diff --git a/src/nodes/insert.h b/src/nodes/insert.h index 580588d899..98a120aa83 100644 --- a/src/nodes/insert.h +++ b/src/nodes/insert.h @@ -5,6 +5,7 @@ #include "base/units.h" #include "classes/moleculeSet.h" +#include "classes/structure.h" #include "nodes/node.h" // Forward Declarations @@ -39,19 +40,34 @@ class InsertNode : public Node // Return enum option info for BoxActionStyle static EnumOptions boxActionStyles(); + // Box Action Style + enum class InstantiationMethod + { + Sample, /* N instances sampled randomly from instances, honouring the specified rotation/translation options */ + InstantiateAll, /* Instantiate all M instances in their current positions */ + }; + // Return enum option info for BoxActionStyle + static EnumOptions instantiationMethod(); + private: + // Typedef for allowed insert types (species/moleculeset) + using InsertTypeVariant = VariantParameterData; + // Insert type input and output + InsertTypeVariant speciesVariant_; // Target configuration to insert into Configuration *configuration_{nullptr}; + // Instances + Structure instances_; // AtomTypes owned by the node const std::vector> *atomTypes_{nullptr}; - // Species to be added (if no MoleculeSet is given) - const Species *species_{nullptr}; - // MoleculeSet to be added (if no Species is given) - const MoleculeSet *moleculeSet_{nullptr}; // The default box action if none is specified static constexpr BoxActionStyle defaultBoxAction_ = BoxActionStyle::AddVolume; + // The default instantiation method if none is specified + static constexpr InstantiationMethod defaultInstantiationMethod_ = InstantiationMethod::InstantiateAll; // Action to take on the Box geometry / volume on addition of the species BoxActionStyle boxAction_{defaultBoxAction_}; + // Strategy for instantiation of species during insertion + InstantiationMethod instantiationMethod_{defaultInstantiationMethod_}; // Target density when adding molecules (if adjusting box size) Number density_{1.0}; // Units for the specified density value diff --git a/src/nodes/parameter.h b/src/nodes/parameter.h index 2fb57ca779..1986b9b566 100644 --- a/src/nodes/parameter.h +++ b/src/nodes/parameter.h @@ -52,6 +52,7 @@ class ParameterBase : public Serialisable ClearData, /* Indicates that any local data should be cleared if the parameter is changed */ Input, /* Indicates that the parameter is meant to be a sink for data and not a source */ Output, /* Indicates that the parameter is meant to be a source of data and not a sink */ + Dynamic, /* Indicates that the parameter is meant to be a dynamic source of data and not a sink */ }; // Allowed Edge Count enum AllowedEdgeCount diff --git a/src/nodes/registry.cpp b/src/nodes/registry.cpp index 695042afa4..1dda84d876 100644 --- a/src/nodes/registry.cpp +++ b/src/nodes/registry.cpp @@ -14,6 +14,7 @@ #include "nodes/configuration.h" #include "nodes/dAngle.h" #include "nodes/derivative.h" +#include "nodes/detectMolecules.h" #include "nodes/dotProduct.h" #include "nodes/edge.h" #include "nodes/energy.h" @@ -92,6 +93,7 @@ void NodeRegistry::instantiateNodeProducers() {"ImportCIFStructure", makeDerivedNode()}, {"DAngle", makeDerivedNode()}, {"Derivative", makeDerivedNode()}, + {"DetectMolecules", makeDerivedNode()}, {"DotProduct", makeDerivedNode()}, {"Energy", makeDerivedNode()}, {"EPSR", makeDerivedNode()}, diff --git a/src/nodes/test.cpp b/src/nodes/test.cpp index 3341632115..7dc5e54568 100644 --- a/src/nodes/test.cpp +++ b/src/nodes/test.cpp @@ -12,6 +12,9 @@ TestNode::TestNode(Graph *parentGraph) : Node(parentGraph) addInput("NumberVector", "A vector of numbers", numberVector_); addInput("OptionalNumber", "A single number", optionalNumber_); addInput("Variant", "A variant", variant_); + addInput("Message", "A message", message_); + addInput("Char", "A character", char_); + addInput("CharPtr", "A character", charPtr_); // Outputs addOutput("Configuration", "A configuration output", configuration_); @@ -54,5 +57,39 @@ NodeConstants::ProcessResult TestNode::process() else optionalConfiguration_ = std::nullopt; + // Standard dynamic outputs + messageParts_.clear(); + messageParts_.insert(messageParts_.end(), message_.begin(), message_.end()); + + /* + * Dynamic outputs + */ + + // Register dynamic (standard) outputs + for (int i = 0; i < messageParts_.size(); i++) + { + auto val = messageParts_[i]; + auto paramName = std::string("Message-Part" + std::format("-{}", i)); + + // Check if output already exists - do not add if it does + if (outputs_.find(paramName) != outputs_.end()) + continue; + + addOutput(paramName, "Part of a message", messageParts_[i]); + } + + // Register dynamic pointer outputs + for (int i = 0; i < messageParts_.size(); i++) + { + auto val = messageParts_[i]; + auto paramName = std::string("Message-Ptr-Part" + std::format("-{}", i)); + + // Check if output already exists - do not add if it does + if (outputs_.find(paramName) != outputs_.end()) + continue; + + addPointerOutput(paramName, "Part of a message", messageParts_[i]); + } + return NodeConstants::ProcessResult::Success; } diff --git a/src/nodes/test.h b/src/nodes/test.h index 1cb3be09da..fe7d3be181 100644 --- a/src/nodes/test.h +++ b/src/nodes/test.h @@ -33,6 +33,11 @@ class TestNode : public Node // Variant using TestVariant = VariantParameterData; TestVariant variant_; + // Test string + char char_; + char *charPtr_; + std::string message_; + std::vector messageParts_; public: // Return type of the node diff --git a/tests/nodes/cif.cpp b/tests/nodes/cif.cpp index 0c7fcc7de0..da91609a1b 100644 --- a/tests/nodes/cif.cpp +++ b/tests/nodes/cif.cpp @@ -3,8 +3,16 @@ #include "classes/configuration.h" #include "classes/empiricalFormula.h" +#include "data/elements.h" +#include "nodes/calculateBonding.h" #include "nodes/cif/importCIFStructure.h" #include "tests/testGraph.h" +#include "nodes/detectMolecules.h" +#include "nodes/supercellConfiguration.h" +#include "tests/graphData.h" +#include "tests/testData.h" +#include +#include #include namespace UnitTest @@ -15,33 +23,102 @@ class CIFNodeTest : public ::testing::Test CIFNodeTest() = default; ~CIFNodeTest() = default; - protected: - TestGraph testGraph_; - const std::string delimiter_{".cif"}; - const std::string path_{"cif/"}; - public: // Molecular species information using MolecularSpeciesInfo = std::tuple; - // Create CIF graph - void createGraph(std::string filename) - { - auto name = cifNameFromFile(filename); - EXPECT_TRUE(testGraph_.appendNode("ImportCIFStructure", name)); - testGraph_.fetchHead()->setOption("FilePath", path_ + filename); - } - // Determine CIF node name from filename - std::string cifNameFromFile(std::string filename) + // Retrieve detected molecule structures + std::vector getDetectedMolecularStructures(const DetectMoleculesNode *node, int N) { - auto name = filename.substr(0, filename.find(delimiter_)); - return name; + std::vector structures; + for (int i = 0; i < N; i++) + { + auto structureI = node->findOutput("DetectedMolecule-" + std::to_string(i)); + structures.push_back(structureI->get()); + } + return structures; } - // Retrieve CIF context by filename - ImportCIFStructureNode *getContextByFileName(std::string filename) + // Extend graph to convert detected species to a supercell configuration + void extendToSupercell(TestGraph *graph, std::vector> expectedSpecies, + const Vector3 &boxLengths, const Vector3 &boxAngles, Vector3i supercellRepeat = {1, 1, 1}) { - auto name = cifNameFromFile(filename); - auto node = testGraph_.findNode(name); - return static_cast(node); + EXPECT_TRUE(graph->appendNode("Configuration")); + EXPECT_TRUE(graph->appendNode("SetBox")); + ASSERT_TRUE(graph->fetchHead()->setOption("Lengths", boxLengths)); + ASSERT_TRUE(graph->fetchHead()->setOption("Angles", boxAngles)); + EXPECT_TRUE(graph->appendNode("SupercellConfiguration")); + ASSERT_TRUE(graph->fetchHead()->setOption("SupercellRepeat", supercellRepeat)); + ASSERT_TRUE(graph->addEdge({"Configuration", "Configuration", "SetBox", "Input"})); + + const auto nExpectedSpecies = expectedSpecies.size(); + + for (const auto &sp : expectedSpecies) + { + auto [z, name] = sp; + EXPECT_TRUE(graph->addNode(TestGraph::createAtomicSpecies(z), name)); + EXPECT_TRUE(graph->appendNode("Insert", std::string("Insert" + name))); + ASSERT_TRUE(graph->fetchHead()->setOption("BoxAction", InsertNode::BoxActionStyle::None)); + } + + // Create species from structure + ASSERT_TRUE(graph->addEdge({"DetectMolecules", "DetectedMolecule-0", expectedSpecies.front().second, "Structure"})); + + // Pass configuration output from set box node to the input configuration of this insert node + ASSERT_TRUE( + graph->addEdge({"SetBox", "Output", std::string("Insert" + expectedSpecies.front().second), "Configuration"})); + + // Pass this species to its insert node + ASSERT_TRUE(graph->addEdge( + {expectedSpecies.front().second, "Species", std::string("Insert" + expectedSpecies.front().second), "Species"})); + + // Pass the corresponding detected molecular structure to this species' insert node + // TODO: check if we have a reliable molecule name to use here at the structure level + ASSERT_TRUE(graph->addEdge( + {"DetectMolecules", "DetectedMolecule-0", std::string("Insert" + expectedSpecies.front().second), "Instances"})); + + for (int i = 1; i < expectedSpecies.size() - 1; i++) + { + auto lastSpeciesName = expectedSpecies[i - 1].second; + auto speciesName = expectedSpecies[i].second; + + // Create species from structure + ASSERT_TRUE(graph->addEdge( + {"DetectMolecules", std::string("DetectedMolecule-" + std::to_string(i)), speciesName, "Structure"})); + + // Pass configuration output from preceding insert node to the input configuration of this one + ASSERT_TRUE(graph->addEdge({std::string("Insert" + lastSpeciesName), "Configuration", + std::string("Insert" + speciesName), "Configuration"})); + + // Pass this species to its insert node + ASSERT_TRUE(graph->addEdge({speciesName, "Species", std::string("Insert" + speciesName), "Species"})); + + // Pass the corresponding detected molecular structure to this species' insert node + // TODO: check if we have a reliable molecule name to use here at the structure level + ASSERT_TRUE(graph->addEdge({"DetectMolecules", std::string("DetectedMolecule-" + std::to_string(i)), + std::string("Insert" + speciesName), "Instances"})); + } + + // + ASSERT_TRUE(graph->addEdge({std::string("Insert" + expectedSpecies[nExpectedSpecies - 2].second), "Configuration", + std::string("Insert" + expectedSpecies.back().second), "Configuration"})); + + // Create species from structure + ASSERT_TRUE( + graph->addEdge({"DetectMolecules", std::string("DetectedMolecule-" + std::to_string(expectedSpecies.size() - 1)), + expectedSpecies.back().second, "Structure"})); + + // Pass configuration output from set box node to the input configuration of the supercell configuration + ASSERT_TRUE(graph->addEdge({std::string("Insert" + expectedSpecies.back().second), "Configuration", + "SupercellConfiguration", "Configuration"})); + + // Pass this species to its insert node + ASSERT_TRUE(graph->addEdge( + {expectedSpecies.back().second, "Species", std::string("Insert" + expectedSpecies.back().second), "Species"})); + + // Pass the corresponding detected molecular structure to this species' insert node + // TODO: check if we have a reliable molecule name to use here at the structure level + ASSERT_TRUE( + graph->addEdge({"DetectMolecules", std::string("DetectedMolecule-" + std::to_string(expectedSpecies.size() - 1)), + std::string("Insert" + expectedSpecies.back().second), "Instances"})); } // Test Box definition void testBox(const Configuration *cfg, const Vector3 &lengths, const Vector3 &angles, int nAtoms) @@ -56,12 +133,13 @@ class CIFNodeTest : public ::testing::Test EXPECT_NEAR(cfg->box().axisAngles().z, angles.z, 1.0e-6); } // Test molecular species information provided - void testMolecularSpecies(const CIFMolecularSpecies &molSp, const MolecularSpeciesInfo &info) + void testDetectedMolecularStructure(const Structure &structure, const MolecularSpeciesInfo &info) { - EXPECT_EQ(molSp.species()->name(), std::get<0>(info)); - EXPECT_EQ(molSp.instances().size(), std::get<1>(info)); - EXPECT_EQ(molSp.species()->nAtoms(), std::get<2>(info)); + // EXPECT_EQ(structure.name(), std::get<0>(info)); + EXPECT_EQ(structure.instances().size(), std::get<1>(info)); + EXPECT_EQ(structure.nAtoms(), std::get<2>(info)); } + /* // Check instance consistency with reference coordinates void testInstanceConsistency(const CIFMolecularSpecies &molSp, const Species &referenceCoordinates) { @@ -76,251 +154,347 @@ class CIFNodeTest : public ::testing::Test // Locate the atom in the reference system at the instance atom coordinates auto instanceR = instanceAtom.r(); auto spAtomIt = std::find_if(referenceCoordinates.atoms().begin(), referenceCoordinates.atoms().end(), - [box, instanceR](const auto &refAtom) - { return box.minimumDistance(refAtom.r(), instanceR) < 0.01; }); + [box, instanceR](const auto &refAtom) + { return box.minimumDistance(refAtom.r(), instanceR) < 0.01; }); std::cout << std::format("{} {} {} {}", Elements::symbol(speciesAtom.Z()), instanceAtom.r().x, - instanceAtom.r().y, instanceAtom.r().z) - << std::endl; + instanceAtom.r().y, instanceAtom.r().z) + << std::endl; ASSERT_NE(spAtomIt, referenceCoordinates.atoms().end()); EXPECT_EQ(spAtomIt->Z(), speciesAtom.Z()); } } } + */ }; TEST_F(CIFNodeTest, Parse) { + TestGraph testGraph; + // Test files with expected number of structure atoms std::vector> cifs = {{"1557470.cif", 86}, {"1557599.cif", 56}, {"7705246.cif", 364}, {"9000004.cif", 6}, {"9000095.cif", 30}, {"9000418.cif", 64}}; for (auto &[cif, nStructureAtoms] : cifs) { - createGraph(cif); - auto node = testGraph_.findNode(cifNameFromFile(cif)); - ASSERT_EQ(node->run(), NodeConstants::ProcessResult::Success); - const auto structure = node->getOutputValue("Structure"); + ASSERT_TRUE(testGraph.appendNode("ImportCIFStructure", cif)); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_EQ(testGraph.fetchHead()->run(), NodeConstants::ProcessResult::Success); + const auto structure = testGraph.fetchHead()->getOutputValue("Structure"); ASSERT_EQ(structure.atoms().size(), nStructureAtoms); } } -/* -TEST_F(CIFNodeTest, NaCl) +TEST_F(CIFNodeTest, NaClContinuous) { + TestGraph testGraph; + // Load the CIF file - auto cif = "NaCl-1000041.cif"; - createGraph(cif); - auto loaderNode = testGraph_.findNode(cifNameFromFile(cif)); - ASSERT_EQ(loaderNode->run(), NodeConstants::ProcessResult::Success); + auto cif = std::string("NaCl-1000041.cif"); - auto cifContext = getContextByFileName(cif); - ASSERT_TRUE(cifContext); - EXPECT_TRUE(cifContext->generate()); + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + EXPECT_TRUE(testGraph.appendNode("CalculateBonding")); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "CalculateBonding", "Structure"}); + testGraph.addEdge({"CalculateBonding", "Structure", "DetectMolecules", "Structure"}); - // Check basic info - auto molecularSpeciesNode = testGraph_.findNode(cifNameFromFile(cif) + "//MolecularSpecies"); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); + // Basic info + ASSERT_EQ(testGraph.findNode("CalculateBonding")->run(), NodeConstants::ProcessResult::Success); + EXPECT_EQ(testGraph.findNode("ImportCIFStructure")->findOption("SpaceGroupID")->get(), + SpaceGroups::SpaceGroup_225); - EXPECT_EQ(cifContext->spaceGroup(), SpaceGroups::SpaceGroup_225); - constexpr double A = 5.62; - testBox(molecularSpeciesNode->getOutputValue("SupercellConfiguration"), {A, A, A}, {90, 90, 90}, 8); + // constexpr double A = 5.62; + + // We should find a continuous framework after rebonding and the detect molecules node should fail accordingly + ASSERT_EQ(testGraph.findNode("DetectMolecules")->run(), NodeConstants::ProcessResult::Failed); +} + +TEST_F(CIFNodeTest, NaClMolecules) +{ + TestGraph testGraph; + + // Load the CIF file + auto cif = std::string("NaCl-1000041.cif"); - // Calculating bonding is the default, but this gives a continuous framework... - EXPECT_EQ(molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies").size(), 0); + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "DetectMolecules", "Structure"}); - // Get molecular species - auto bondingNode = testGraph_.findNode(cifNameFromFile(cif) + "//BondingOptions"); - bondingNode->setOption("UseCIFBondingDefinitions", true); - testGraph_.setUpdateRequired(); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); + auto detectMoleculesNode = static_cast(testGraph.findNode("DetectMolecules")); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); - auto molecularSpecies = molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies"); + // Basic info + EXPECT_EQ(testGraph.findNode("ImportCIFStructure")->findOption("SpaceGroupID")->get(), + SpaceGroups::SpaceGroup_225); + constexpr double A = 5.62; - EXPECT_EQ(molecularSpecies.size(), 2); - testMolecularSpecies(molecularSpecies.at(0), {"Na", 4, 1}); + // Check atomic positions std::vector R = {{0.0, 0.0, 0.0}, {0.0, A / 2, A / 2}, {A / 2, 0.0, A / 2}, {A / 2, A / 2, 0.0}}; - for (auto &&[instance, r2] : zip(molecularSpecies.at(0).instances(), R)) - testVector3(instance.localAtoms()[0].r(), r2); - testMolecularSpecies(molecularSpecies.at(1), {"Cl", 4, 1}); - for (auto &&[instance, r2] : zip(molecularSpecies.at(1).instances(), R)) - testVector3(instance.localAtoms()[0].r(), (r2 - A / 2).abs()); + auto structures = getDetectedMolecularStructures(detectMoleculesNode, 2); + EXPECT_EQ(structures.size(), 2); + testDetectedMolecularStructure(structures.at(0), {"Na", 4, 1}); + for (auto &&[instance, r2] : zip(structures.at(0).instances(), R)) + DissolveSystemTest::checkVec3(instance[0], r2); + testDetectedMolecularStructure(structures.at(1), {"Cl", 4, 1}); + for (auto &&[instance, r2] : zip(structures.at(1).instances(), R)) + DissolveSystemTest::checkVec3(instance[0], (r2 - A / 2).abs()); // 2x2x2 supercell - molecularSpeciesNode->setOption("SupercellRepeat", {2, 2, 2}); - testGraph_.dissolveGraph()->setUpdateRequired(); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); - testBox(molecularSpeciesNode->getOutputValue("SupercellConfiguration"), {A * 2, A * 2, A * 2}, + extendToSupercell(&testGraph, {{Elements::Na, "Na"}, {Elements::Cl, "Cl"}}, {A, A, A}, {90, 90, 90}, {2, 2, 2}); + auto supercellConfigurationNode = static_cast(testGraph.findNode("SupercellConfiguration")); + ASSERT_EQ(supercellConfigurationNode->run(), NodeConstants::ProcessResult::Success); + testBox(supercellConfigurationNode->getOutputValue("SupercellConfiguration"), {A * 2, A * 2, A * 2}, {90, 90, 90}, 8 * 8); } TEST_F(CIFNodeTest, NaClO3) { + TestGraph testGraph; + // Load the CIF file - auto cif = "NaClO3-1010057.cif"; - createGraph(cif); - ASSERT_EQ(testGraph_.findNode(cifNameFromFile(cif))->run(), NodeConstants::ProcessResult::Success); + auto cif = std::string("NaClO3-1010057.cif"); + + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "DetectMolecules", "Structure"}); - auto cifContext = getContextByFileName(cif); - ASSERT_TRUE(cifContext); - EXPECT_TRUE(cifContext->generate()); + ASSERT_EQ(testGraph.findNode("ImportCIFStructure")->run(), NodeConstants::ProcessResult::Success); // Check basic info - auto molecularSpeciesNode = testGraph_.findNode(cifNameFromFile(cif) + "//MolecularSpecies"); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); + auto detectMoleculesNode = static_cast(testGraph.findNode("DetectMolecules")); - EXPECT_EQ(cifContext->spaceGroup(), SpaceGroups::SpaceGroup_198); - constexpr double A = 6.55; - testBox(molecularSpeciesNode->getOutputValue("SupercellConfiguration"), {A, A, A}, {90, 90, 90}, 20); + EXPECT_EQ(testGraph.findNode("ImportCIFStructure")->findOption("SpaceGroupID")->get(), + SpaceGroups::SpaceGroup_198); - // Turn off automatic bond calculation - there are no bonding defs in the CIF, so we expect species for each atomic + // No bonding defs in the CIF, so we expect species for each atomic // component (4 Na, 4 Cl, and 12 O) - auto bondingNode = testGraph_.findNode(cifNameFromFile(cif) + "//BondingOptions"); - bondingNode->setOption("UseCIFBondingDefinitions", true); - testGraph_.setUpdateRequired(); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); - auto cifMolsA = molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies"); - ASSERT_EQ(cifMolsA.size(), 3); - testMolecularSpecies(cifMolsA.at(0), {"Na", 4, 1}); - testMolecularSpecies(cifMolsA.at(1), {"Cl", 4, 1}); - testMolecularSpecies(cifMolsA.at(2), {"O", 12, 1}); + auto detectedMoleculeStructuresA = getDetectedMolecularStructures(detectMoleculesNode, 3); + testDetectedMolecularStructure(detectedMoleculeStructuresA.at(0), {"Na", 4, 1}); + testDetectedMolecularStructure(detectedMoleculeStructuresA.at(1), {"Cl", 4, 1}); + testDetectedMolecularStructure(detectedMoleculeStructuresA.at(2), {"O", 12, 1}); + + // Check box + constexpr double A = 6.55; + extendToSupercell(&testGraph, {{Elements::Na, "Na"}, {Elements::Cl, "Cl"}, {Elements::O, "O"}}, {A, A, A}, {90, 90, 90}); + auto supercellConfigurationNode = static_cast(testGraph.findNode("SupercellConfiguration")); + ASSERT_EQ(supercellConfigurationNode->run(), NodeConstants::ProcessResult::Success); + testBox(supercellConfigurationNode->getOutputValue("SupercellConfiguration"), {A, A, A}, {90, 90, 90}, 20); // Calculate bonding ourselves to get the correct species - bondingNode->setOption("UseCIFBondingDefinitions", false); - testGraph_.setUpdateRequired(); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); - auto cifMolsB = molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies"); - ASSERT_EQ(cifMolsB.size(), 2); - testMolecularSpecies(cifMolsB.at(0), {"Na", 4, 1}); - testMolecularSpecies(cifMolsB.at(1), {"ClO3", 4, 4}); + EXPECT_TRUE(testGraph.appendNode("CalculateBonding")); + testGraph.removeEdge({"ImportCIFStructure", "Structure", "DetectMolecules", "Structure"}); + testGraph.addEdge({"ImportCIFStructure", "Structure", "CalculateBonding", "Structure"}); + testGraph.addEdge({"CalculateBonding", "Structure", "DetectMolecules", "Structure"}); + + testGraph.setUpdateRequired(); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); + auto detectedMoleculeStructuresB = getDetectedMolecularStructures(detectMoleculesNode, 2); + ASSERT_EQ(detectedMoleculeStructuresB.size(), 2); + testDetectedMolecularStructure(detectedMoleculeStructuresB.at(0), {"Na", 4, 1}); + testDetectedMolecularStructure(detectedMoleculeStructuresB.at(1), {"ClO3", 4, 4}); } TEST_F(CIFNodeTest, CuBTC) { + TestGraph testGraph; + // Load the CIF file - auto cif = "CuBTC-7108574.cif"; - createGraph(cif); - ASSERT_EQ(testGraph_.findNode(cifNameFromFile(cif))->run(), NodeConstants::ProcessResult::Success); + auto cif = std::string("CuBTC-7108574.cif"); + + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_TRUE(testGraph.appendNode("CalculateBonding")); + testGraph.fetchHead()->setOption("Clear", true); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "CalculateBonding", "Structure"}); + testGraph.addEdge({"CalculateBonding", "Structure", "DetectMolecules", "Structure"}); - auto cifContext = getContextByFileName(cif); - ASSERT_TRUE(cifContext); - EXPECT_TRUE(cifContext->generate()); + ASSERT_EQ(testGraph.findNode("ImportCIFStructure")->run(), NodeConstants::ProcessResult::Success); // Check basic info - auto molecularSpeciesNode = testGraph_.findNode(cifNameFromFile(cif) + "//MolecularSpecies"); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); + auto detectMoleculesNode = static_cast(testGraph.findNode("DetectMolecules")); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); + + auto detectedMoleculeStructures = getDetectedMolecularStructures(detectMoleculesNode, 2); + EXPECT_EQ(detectedMoleculeStructures.size(), 2); + const auto box = detectedMoleculeStructures[0].box(); + + EXPECT_EQ(testGraph.findNode("ImportCIFStructure")->findOption("SpaceGroupID")->get(), + SpaceGroups::SpaceGroup_225); - EXPECT_EQ(cifContext->spaceGroup(), SpaceGroups::SpaceGroup_225); + /* TODO: Handle supercell configurations constexpr auto A = 26.3336; - testBox(molecularSpeciesNode->getOutputValue("SupercellConfiguration"), {A, A, A}, {90, 90, 90}, 672); + // testBox(detectMoleculesNode->getOutputValue("SupercellConfiguration"), {A, A, A}, {90, 90, 90}, + 672); + + // Check basic formula (which includes bound water oxygens - with no H - at this point) and using O group // 16 basic formula units per unit cell constexpr auto N = 16; - - // Check basic formula (which includes bound water oxygens - with no H - at this point) and using O group EmpiricalFormula::EmpiricalFormulaMap cellFormulaH = { {Elements::Cu, 3 * N}, {Elements::C, 18 * N}, {Elements::H, 6 * N}, {Elements::O, 15 * N}}; - EXPECT_EQ( - EmpiricalFormula::formula(molecularSpeciesNode->getOutputValue("SupercellConfiguration")->atoms(), - [](const auto &i) { return i.speciesAtom()->Z(); }), - EmpiricalFormula::formula(cellFormulaH)); - auto cifMolsA = molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies"); - EXPECT_EQ(cifMolsA.size(), 2); - + EXPECT_EQ(EmpiricalFormula::formula(detectMoleculesNode->getOutputValue("SupercellConfiguration")->atoms(), + [](const auto &i) { return i.speciesAtom()->Z(); }), + EmpiricalFormula::formula(cellFormulaH)); + */ +} +/* +TEST_F(CIFNodeTest, CuBTCActiveAssemblies) +{ + TestGraph testGraph; // Change active assemblies to get amine-substituted structure + EmpiricalFormula::EmpiricalFormulaMap cellFormulaNH2 = cellFormulaH; cellFormulaNH2[Elements::N] = 6 * N; cellFormulaNH2[Elements::H] *= 2; - EXPECT_TRUE(testGraph_.appendNode("SetCIFAtomGroupActivity", cifNameFromFile(cif) + "//AtomGroupA1")); - testGraph_.fetchHead()->setOption("Assembly", std::string("A")); - testGraph_.fetchHead()->setOption("AtomGroup", std::string("1")); - testGraph_.fetchHead()->setOption("SetActive", false); - EXPECT_TRUE(testGraph_.appendNode("SetCIFAtomGroupActivity", cifNameFromFile(cif) + "//AtomGroupB2")); - testGraph_.fetchHead()->setOption("Assembly", std::string("B")); - testGraph_.fetchHead()->setOption("AtomGroup", std::string("2")); - testGraph_.fetchHead()->setOption("SetActive", true); - EXPECT_TRUE(testGraph_.appendNode("SetCIFAtomGroupActivity", cifNameFromFile(cif) + "//AtomGroupC2")); - testGraph_.fetchHead()->setOption("Assembly", std::string("C")); - testGraph_.fetchHead()->setOption("AtomGroup", std::string("2")); - testGraph_.fetchHead()->setOption("SetActive", true); - testGraph_.removeEdge( - {cifNameFromFile(cif) + "//StructureCleanup", "CIFContext", std::string(molecularSpeciesNode->name()), "CIFContext"}); - testGraph_.addEdge( - {cifNameFromFile(cif) + "//StructureCleanup", "CIFContext", cifNameFromFile(cif) + "//AtomGroupA1", "CIFContext"}); - testGraph_.addEdge( - {cifNameFromFile(cif) + "//AtomGroupA1", "CIFContext", cifNameFromFile(cif) + "//AtomGroupB2", "CIFContext"}); - testGraph_.addEdge( - {cifNameFromFile(cif) + "//AtomGroupB2", "CIFContext", cifNameFromFile(cif) + "//AtomGroupC2", "CIFContext"}); - testGraph_.addEdge( - {cifNameFromFile(cif) + "//AtomGroupC2", "CIFContext", std::string(molecularSpeciesNode->name()), "CIFContext"}); - testGraph_.setUpdateRequired(); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); - EXPECT_EQ( - EmpiricalFormula::formula(molecularSpeciesNode->getOutputValue("SupercellConfiguration")->atoms(), - [](const auto &i) { return i.speciesAtom()->Z(); }), - EmpiricalFormula::formula(cellFormulaNH2)); + EXPECT_TRUE(testGraph.appendNode("SetCIFAtomGroupActivity", cifNameFromFile(cif) + "//AtomGroupA1")); + testGraph.fetchHead()->setOption("Assembly", std::string("A")); + testGraph.fetchHead()->setOption("AtomGroup", std::string("1")); + testGraph.fetchHead()->setOption("SetActive", false); + EXPECT_TRUE(testGraph.appendNode("SetCIFAtomGroupActivity", cifNameFromFile(cif) + "//AtomGroupB2")); + testGraph.fetchHead()->setOption("Assembly", std::string("B")); + testGraph.fetchHead()->setOption("AtomGroup", std::string("2")); + testGraph.fetchHead()->setOption("SetActive", true); + EXPECT_TRUE(testGraph.appendNode("SetCIFAtomGroupActivity", cifNameFromFile(cif) + "//AtomGroupC2")); + testGraph.fetchHead()->setOption("Assembly", std::string("C")); + testGraph.fetchHead()->setOption("AtomGroup", std::string("2")); + testGraph.fetchHead()->setOption("SetActive", true); + testGraph.removeEdge( + {cifNameFromFile(cif) + "//StructureCleanup", "CIFContext", std::string(detectMoleculesNode->name()), +"CIFContext"}); testGraph.addEdge( {cifNameFromFile(cif) + "//StructureCleanup", "CIFContext", cifNameFromFile(cif) + +"//AtomGroupA1", "CIFContext"}); testGraph.addEdge( {cifNameFromFile(cif) + "//AtomGroupA1", "CIFContext", +cifNameFromFile(cif) + "//AtomGroupB2", "CIFContext"}); testGraph.addEdge( {cifNameFromFile(cif) + "//AtomGroupB2", +"CIFContext", cifNameFromFile(cif) + "//AtomGroupC2", "CIFContext"}); testGraph.addEdge( {cifNameFromFile(cif) + +"//AtomGroupC2", "CIFContext", std::string(detectMoleculesNode->name()), "CIFContext"}); testGraph.setUpdateRequired(); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); + EXPECT_EQ(EmpiricalFormula::formula(detectMoleculesNode->getOutputValue("SupercellConfiguration")->atoms(), + [](const auto &i) { return i.speciesAtom()->Z(); }), + EmpiricalFormula::formula(cellFormulaNH2)); // Remove those free oxygens so we just have a framework - auto removeAtomicsNode = testGraph_.findNode(cifNameFromFile(cif) + "//RemoveAtomic"); + auto removeAtomicsNode = testGraph.findNode(cifNameFromFile(cif) + "//RemoveAtomic"); removeAtomicsNode->setOption("RemoveAtomics", true); - testGraph_.setUpdateRequired(); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); - auto cifMolsB = molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies"); - EXPECT_EQ(cifMolsB.size(), 0); + testGraph.setUpdateRequired(); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); + auto detectedMoleculeStructuresB = getDetectedMolecularStructures(detectMoleculesNode, 2); + EXPECT_EQ(detectedMoleculeStructuresB.size(), 0); + } +*/ -TEST_F(CIFNodeTest, MoleculeOrdering) +TEST_F(CIFNodeTest, MoleculeOrderingSimple) { - const auto cifFiles = {"molecule-test-simple-ordered.cif", "molecule-test-simple-unordered.cif", - "molecule-test-simple-unordered-rotated.cif"}; - for (auto cifFile : cifFiles) - { - // Load the CIF file - createGraph(cifFile); - ASSERT_EQ(testGraph_.findNode(cifNameFromFile(cifFile))->run(), NodeConstants::ProcessResult::Success); + TestGraph testGraph; - auto cifContext = getContextByFileName(cifFile); - ASSERT_TRUE(cifContext); - EXPECT_TRUE(cifContext->generate()); + auto cif = std::string("molecule-test-simple-ordered.cif"); - auto molecularSpeciesNode = testGraph_.findNode(cifNameFromFile(cifFile) + "//MolecularSpecies"); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_TRUE(testGraph.appendNode("CalculateBonding")); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules", "DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "CalculateBonding", "Structure"}); + testGraph.addEdge({"CalculateBonding", "Structure", "DetectMolecules", "Structure"}); - auto molecularSpecies = - molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies"); - EXPECT_EQ(molecularSpecies.size(), 1); + auto detectMoleculesNode = static_cast(testGraph.findNode("DetectMolecules")); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); - auto &cifMolecule = molecularSpecies.front(); - EmpiricalFormula::EmpiricalFormulaMap moleculeFormula = { - {Elements::Cl, 1}, {Elements::O, 1}, {Elements::C, 1}, {Elements::H, 3}}; - testMolecularSpecies(cifMolecule, {EmpiricalFormula::formula(moleculeFormula), 6, 6}); + auto detectedMoleculeStructures = getDetectedMolecularStructures(detectMoleculesNode, 1); + EXPECT_EQ(detectedMoleculeStructures.size(), 1); - auto &unitCellSpecies = static_cast(molecularSpeciesNode)->cleanedUnitCellSpecies(); - testInstanceConsistency(cifMolecule, unitCellSpecies); - } + auto &molStructure = detectedMoleculeStructures.front(); + EmpiricalFormula::EmpiricalFormulaMap moleculeFormula = { + {Elements::Cl, 1}, {Elements::O, 1}, {Elements::C, 1}, {Elements::H, 3}}; + testDetectedMolecularStructure(molStructure, {EmpiricalFormula::formula(moleculeFormula), 6, 6}); + + // auto &unitCellSpecies = static_cast(detectMoleculesNode)->cleanedUnitCellSpecies(); + // testInstanceConsistency(cifMolecule, unitCellSpecies); +} + +TEST_F(CIFNodeTest, MoleculeOrderingSimpleUnordered) +{ + TestGraph testGraph; + + auto cif = std::string("molecule-test-simple-ordered.cif"); + + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_TRUE(testGraph.appendNode("CalculateBonding")); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules", "DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "CalculateBonding", "Structure"}); + testGraph.addEdge({"CalculateBonding", "Structure", "DetectMolecules", "Structure"}); + + auto detectMoleculesNode = static_cast(testGraph.findNode("DetectMolecules")); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); + + auto detectedMoleculeStructures = getDetectedMolecularStructures(detectMoleculesNode, 1); + EXPECT_EQ(detectedMoleculeStructures.size(), 1); + + auto &molStructure = detectedMoleculeStructures.front(); + EmpiricalFormula::EmpiricalFormulaMap moleculeFormula = { + {Elements::Cl, 1}, {Elements::O, 1}, {Elements::C, 1}, {Elements::H, 3}}; + testDetectedMolecularStructure(molStructure, {EmpiricalFormula::formula(moleculeFormula), 6, 6}); + + // auto &unitCellSpecies = static_cast(detectMoleculesNode)->cleanedUnitCellSpecies(); + // testInstanceConsistency(cifMolecule, unitCellSpecies); +} + +TEST_F(CIFNodeTest, MoleculeOrderingSimpleUnorderedRotated) +{ + TestGraph testGraph; + + auto cif = std::string("molecule-test-simple-unordered-rotated.cif"); + + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_TRUE(testGraph.appendNode("CalculateBonding")); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules", "DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "CalculateBonding", "Structure"}); + testGraph.addEdge({"CalculateBonding", "Structure", "DetectMolecules", "Structure"}); + + auto detectMoleculesNode = static_cast(testGraph.findNode("DetectMolecules")); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); + + auto detectedMoleculeStructures = getDetectedMolecularStructures(detectMoleculesNode, 1); + EXPECT_EQ(detectedMoleculeStructures.size(), 1); + + auto &molStructure = detectedMoleculeStructures.front(); + EmpiricalFormula::EmpiricalFormulaMap moleculeFormula = { + {Elements::Cl, 1}, {Elements::O, 1}, {Elements::C, 1}, {Elements::H, 3}}; + testDetectedMolecularStructure(molStructure, {EmpiricalFormula::formula(moleculeFormula), 6, 6}); + + // auto &unitCellSpecies = static_cast(detectMoleculesNode)->cleanedUnitCellSpecies(); + // testInstanceConsistency(cifMolecule, unitCellSpecies); } TEST_F(CIFNodeTest, BigMoleculeOrdering) { - const auto cifFile = "Bisphen_n_arenes_1517789.cif"; - createGraph(cifFile); - ASSERT_EQ(testGraph_.findNode(cifNameFromFile(cifFile))->run(), NodeConstants::ProcessResult::Success); + TestGraph testGraph; + + const auto cif = std::string("Bisphen_n_arenes_1517789.cif"); - auto cifContext = getContextByFileName(cifFile); - ASSERT_TRUE(cifContext); - EXPECT_TRUE(cifContext->generate()); + EXPECT_TRUE(testGraph.appendNode("ImportCIFStructure")); + testGraph.fetchHead()->setOption("FilePath", "cif/" + cif); + ASSERT_TRUE(testGraph.appendNode("DetectMolecules", "DetectMolecules")); + testGraph.addEdge({"ImportCIFStructure", "Structure", "DetectMolecules", "Structure"}); - auto molecularSpeciesNode = testGraph_.findNode(cifNameFromFile(cifFile) + "//MolecularSpecies"); - ASSERT_EQ(molecularSpeciesNode->run(), NodeConstants::ProcessResult::Success); + auto detectMoleculesNode = static_cast(testGraph.findNode("DetectMolecules")); + ASSERT_EQ(detectMoleculesNode->run(), NodeConstants::ProcessResult::Success); - auto molecularSpecies = molecularSpeciesNode->getOutputValue>("DetectedMolecularSpecies"); - EXPECT_EQ(molecularSpecies.size(), 1); + auto detectedStructures = getDetectedMolecularStructures(detectMoleculesNode, 1); + EXPECT_EQ(detectedStructures.size(), 1); - auto &cifMolecule = molecularSpecies.front(); + auto &molStructure = detectedStructures.front(); EmpiricalFormula::EmpiricalFormulaMap moleculeFormula = {{Elements::O, 6}, {Elements::C, 51}, {Elements::H, 54}}; - testMolecularSpecies(cifMolecule, {EmpiricalFormula::formula(moleculeFormula), 4, 111}); + testDetectedMolecularStructure(molStructure, {EmpiricalFormula::formula(moleculeFormula), 4, 111}); - auto &unitCellSpecies = static_cast(molecularSpeciesNode)->cleanedUnitCellSpecies(); - testInstanceConsistency(cifMolecule, unitCellSpecies); + // auto &unitCellSpecies = static_cast(detectMoleculesNode)->cleanedUnitCellSpecies(); + // testInstanceConsistency(cifMolecule, unitCellSpecies); } -*/ -} // namespace UnitTest + +} // namespace UnitTest \ No newline at end of file diff --git a/tests/nodes/parameters.cpp b/tests/nodes/parameters.cpp index 1652e2cd9a..95165b3634 100644 --- a/tests/nodes/parameters.cpp +++ b/tests/nodes/parameters.cpp @@ -359,4 +359,70 @@ TEST_F(ParametersTest, OptionalPointerToVariant) EXPECT_EQ(std::get(b_->variant().data), &a_->optionalConfiguration().value()); } +TEST_F(ParametersTest, DynamicOutput) +{ + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "Sender")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverA")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverB")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverC")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverD")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverE")); + + auto sender = testGraph_.findNode("Sender"); + ASSERT_TRUE(sender->setInput("Message", std::string("hello"))); + ASSERT_EQ(testGraph_.runDynamic(sender, + { + {"Sender", "Message-Part-0", "RecieverA", "Char"}, + {"Sender", "Message-Part-1", "RecieverB", "Char"}, + {"Sender", "Message-Part-2", "RecieverC", "Char"}, + {"Sender", "Message-Part-3", "RecieverD", "Char"}, + {"Sender", "Message-Part-4", "RecieverE", "Char"}, + + }), + NodeConstants::ProcessResult::Success); + + std::vector chars; + for (const auto &which : {"A", "B", "C", "D", "E"}) + { + auto node = testGraph_.findNode("Reciever" + std::string(which)); + chars.push_back(node->findInput("Char")->get()); + } + + std::string message(chars.begin(), chars.end()); + ASSERT_EQ(message, "hello"); +} + +TEST_F(ParametersTest, DynamicPointerOutput) +{ + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "Sender")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverA")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverB")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverC")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverD")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverE")); + + auto sender = testGraph_.findNode("Sender"); + ASSERT_TRUE(sender->setInput("Message", std::string("hello"))); + ASSERT_EQ(testGraph_.runDynamic(sender, + { + {"Sender", "Message-Ptr-Part-0", "RecieverA", "CharPtr"}, + {"Sender", "Message-Ptr-Part-1", "RecieverB", "CharPtr"}, + {"Sender", "Message-Ptr-Part-2", "RecieverC", "CharPtr"}, + {"Sender", "Message-Ptr-Part-3", "RecieverD", "CharPtr"}, + {"Sender", "Message-Ptr-Part-4", "RecieverE", "CharPtr"}, + + }), + NodeConstants::ProcessResult::Success); + + std::vector chars; + for (const auto &which : {"A", "B", "C", "D", "E"}) + { + auto node = testGraph_.findNode("Reciever" + std::string(which)); + chars.push_back(*node->findInput("CharPtr")->get()); + } + + std::string message(chars.begin(), chars.end()); + ASSERT_EQ(message, "hello"); +} + } // namespace UnitTest From 9a6946bf6215c5052112242670ddd618d9762652 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 9 Jul 2026 10:04:13 +0100 Subject: [PATCH 2/9] tidy comments --- src/nodes/detectMolecules.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/nodes/detectMolecules.cpp b/src/nodes/detectMolecules.cpp index 8f41dc1b76..c2e278af6a 100644 --- a/src/nodes/detectMolecules.cpp +++ b/src/nodes/detectMolecules.cpp @@ -21,7 +21,7 @@ DetectMoleculesNode::DetectMoleculesNode(Graph *parentGraph) : Node(parentGraph) std::string_view DetectMoleculesNode::type() const { return "DetectMolecules"; } -std::string_view DetectMoleculesNode::summary() const { return "Detect molecular species within a structure"; } +std::string_view DetectMoleculesNode::summary() const { return "Detect molecular instances within a structure"; } /* * Processing @@ -52,7 +52,7 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() return false; }; - // Try selecting within the species from the first atom - if this captures all atoms we have a bound framework... + // Try selecting within the fragment from the first atom - if this captures all atoms we have a bound framework... if (fragmentMap.contains(inputStructure_.nAtoms())) return error( "Can't create molecular definitions since this unit cell appears to be a continuous framework/network. Consider " @@ -111,7 +111,7 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() Flags(NETADefinition::NETACreationFlags::ExplicitHydrogens, NETADefinition::NETACreationFlags::IncludeRootElement)); - // Apply this match over the whole species + // Apply this match over the whole fragment std::vector currentRootAtoms; for (auto fragmentAtomIndex : fragment) { @@ -144,6 +144,7 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() * Get instances */ + // Get all atoms belonging to fragments from the same fragment size group auto fragmentSizeGroupAtoms = getFragmentAtoms(inputStructure_, fragments); // Iterate over all structural atoms, matching their unit cell atoms by NETA From 44c7431822044ef4873966e55861d59334e205a1 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 9 Jul 2026 11:00:55 +0100 Subject: [PATCH 3/9] fix rebase problems --- src/nodes/detectMolecules.cpp | 1 + tests/nodes/cif.cpp | 13 +++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/nodes/detectMolecules.cpp b/src/nodes/detectMolecules.cpp index c2e278af6a..7c5ac3f10b 100644 --- a/src/nodes/detectMolecules.cpp +++ b/src/nodes/detectMolecules.cpp @@ -8,6 +8,7 @@ #include "classes/species.h" #include #include +#include DetectMoleculesNode::DetectMoleculesNode(Graph *parentGraph) : Node(parentGraph) { diff --git a/tests/nodes/cif.cpp b/tests/nodes/cif.cpp index da91609a1b..2a8cc6a57e 100644 --- a/tests/nodes/cif.cpp +++ b/tests/nodes/cif.cpp @@ -6,11 +6,10 @@ #include "data/elements.h" #include "nodes/calculateBonding.h" #include "nodes/cif/importCIFStructure.h" -#include "tests/testGraph.h" #include "nodes/detectMolecules.h" +#include "nodes/species.h" #include "nodes/supercellConfiguration.h" -#include "tests/graphData.h" -#include "tests/testData.h" +#include "tests/testGraph.h" #include #include #include @@ -54,7 +53,9 @@ class CIFNodeTest : public ::testing::Test for (const auto &sp : expectedSpecies) { auto [z, name] = sp; - EXPECT_TRUE(graph->addNode(TestGraph::createAtomicSpecies(z), name)); + std::unique_ptr speciesUnique; + speciesUnique = TestGraph::createAtomicSpecies(z); + EXPECT_TRUE(graph->addNode(std::move(speciesUnique), name)); EXPECT_TRUE(graph->appendNode("Insert", std::string("Insert" + name))); ASSERT_TRUE(graph->fetchHead()->setOption("BoxAction", InsertNode::BoxActionStyle::None)); } @@ -235,10 +236,10 @@ TEST_F(CIFNodeTest, NaClMolecules) EXPECT_EQ(structures.size(), 2); testDetectedMolecularStructure(structures.at(0), {"Na", 4, 1}); for (auto &&[instance, r2] : zip(structures.at(0).instances(), R)) - DissolveSystemTest::checkVec3(instance[0], r2); + testVector3("Molecular instance coordinates", instance[0], r2); testDetectedMolecularStructure(structures.at(1), {"Cl", 4, 1}); for (auto &&[instance, r2] : zip(structures.at(1).instances(), R)) - DissolveSystemTest::checkVec3(instance[0], (r2 - A / 2).abs()); + testVector3("Molecular instance coordinates", instance[0], (r2 - A / 2).abs()); // 2x2x2 supercell extendToSupercell(&testGraph, {{Elements::Na, "Na"}, {Elements::Cl, "Cl"}}, {A, A, A}, {90, 90, 90}, {2, 2, 2}); From 6a01edc3722b29c570a760fc96b96d9091f7236c Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 9 Jul 2026 11:08:36 +0100 Subject: [PATCH 4/9] use fragment atoms directly in loop --- src/nodes/detectMolecules.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/nodes/detectMolecules.cpp b/src/nodes/detectMolecules.cpp index 7c5ac3f10b..79bfddbc36 100644 --- a/src/nodes/detectMolecules.cpp +++ b/src/nodes/detectMolecules.cpp @@ -97,7 +97,7 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() // Set up the return value and bind its contents NETADefinition bestNETA; - std::vector rootAtoms; + std::vector rootAtoms; // Maintain a set of atoms matched by any NETA description we generate std::set alreadyMatched; @@ -113,10 +113,9 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() NETADefinition::NETACreationFlags::IncludeRootElement)); // Apply this match over the whole fragment - std::vector currentRootAtoms; - for (auto fragmentAtomIndex : fragment) + std::vector currentRootAtoms; + for (auto fragmentAtom : fragmentAtoms) { - const auto fragmentAtom = inputStructure_.atom(fragmentAtomIndex); if (neta.matches(fragmentAtom)) { currentRootAtoms.push_back(fragmentAtom); From 7beccb85ceb2106890bf167c1f1f28e697c36b91 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 9 Jul 2026 13:58:09 +0100 Subject: [PATCH 5/9] add in missing function --- tests/testGraph.cpp | 17 +++++++++++++++++ tests/testGraph.h | 3 +++ 2 files changed, 20 insertions(+) diff --git a/tests/testGraph.cpp b/tests/testGraph.cpp index 57f0d3a5ca..0147b65ac4 100644 --- a/tests/testGraph.cpp +++ b/tests/testGraph.cpp @@ -27,6 +27,23 @@ namespace UnitTest Node *TestGraph::fetchHead() const { return head_; } // Returns the name of the current head node in the graph std::string TestGraph::fetchHeadName() const { return head_ ? std::string(head_->name()) : "NO_NODE"; } +// Run the graph in a piecewise manner - initially from a specific node, then from the last node - in order to emplace a set +// of dynamic edges that we expect to exist at run time +NodeConstants::ProcessResult TestGraph::runDynamic(Node *startNode, std::vector edges) const +{ + currentGraph_->setUpdateRequired(); + + auto result = NodeConstants::ProcessResult::Unchanged; + result = startNode->run(); + if (result == NodeConstants::ProcessResult::Failed) + return result; + + for (const auto &edge : edges) + if (!currentGraph_->addEdge(edge) || + currentGraph_->findNode(edge.targetNode)->run() == NodeConstants::ProcessResult::Failed) + return NodeConstants::ProcessResult::Failed; + return result; +} // Append new node to the graph Node *TestGraph::appendNode(const std::string &nodeType, const std::optional &name) { diff --git a/tests/testGraph.h b/tests/testGraph.h index 667d3cdb8c..1ebab54532 100644 --- a/tests/testGraph.h +++ b/tests/testGraph.h @@ -46,6 +46,9 @@ class TestGraph : public DissolveGraph std::string fetchHeadName() const; // Returns reference to current top node in graph, cast to the known node type template NodeType *head() const { return static_cast(head_); } + // Run the graph in a piecewise manner - initially from a specific node, then from the last node - in order to emplace a set + // of dynamic edges that we expect to exist at run time + NodeConstants::ProcessResult runDynamic(Node *startNode, std::vector edges) const; // Append new node to the graph Node *appendNode(const std::string &nodeType, const std::optional &name = {}); // Create species insertion node chain From 8ec0880250bd993c3fffc190cb61daba96b22960 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Fri, 10 Jul 2026 09:28:34 +0100 Subject: [PATCH 6/9] pr comments --- src/classes/structure.cpp | 2 +- src/nodes/detectMolecules.cpp | 253 +++++++++++++++++----------------- src/nodes/detectMolecules.h | 19 +-- src/nodes/insert.cpp | 50 ++----- src/nodes/parameter.h | 1 - tests/nodes/parameters.cpp | 44 +++--- 6 files changed, 174 insertions(+), 195 deletions(-) diff --git a/src/classes/structure.cpp b/src/classes/structure.cpp index 261afcbd10..f1ce45786e 100644 --- a/src/classes/structure.cpp +++ b/src/classes/structure.cpp @@ -124,7 +124,7 @@ const StructureAtom *Structure::atom(int i) const { return atoms_[i].get(); } const std::vector> &Structure::atoms() const { return atoms_; } std::vector> &Structure::atoms() { return atoms_; } -// Return molecular species coordinates +// Return positional instances const std::vector> &Structure::instances() const { return instances_; } std::vector> &Structure::instances() { return instances_; } diff --git a/src/nodes/detectMolecules.cpp b/src/nodes/detectMolecules.cpp index 79bfddbc36..b9f5057b44 100644 --- a/src/nodes/detectMolecules.cpp +++ b/src/nodes/detectMolecules.cpp @@ -37,23 +37,9 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() inputStructure_.unFold(); // Return all discovered molecular fragment index vectors - auto fragmentMap = findMolecularFragments(inputStructure_); + auto fragmentMap = findMolecularFragments(); - // Define lambda for capturing any molecular instances we detect - auto appendInstances = [&](std::vector> &instances, Structure &detectedMolecularStructure, - const std::vector &fragment) -> bool - { - if (!instances.empty()) - { - detectedStructures_.emplace_back(copyStructureAtomsAndBonds(inputStructure_, detectedMolecularStructure, fragment)) - .instances() = instances; - - return true; - } - return false; - }; - - // Try selecting within the fragment from the first atom - if this captures all atoms we have a bound framework... + // Check for a single, bound framework fragment if (fragmentMap.contains(inputStructure_.nAtoms())) return error( "Can't create molecular definitions since this unit cell appears to be a continuous framework/network. Consider " @@ -62,7 +48,7 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() std::set atomMask; for (const auto &[size, fragments] : fragmentMap) - for (const auto &fragment : fragments) + for (const auto &[neta, fragment] : fragments) { // Create a provisional structure for the detected fragment Structure detectedStructure; @@ -70,112 +56,56 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() std::vector> instances; // Get fragment atoms - auto fragmentAtoms = getFragmentAtoms(inputStructure_, fragment); + auto fragmentAtoms = getFragmentAtoms(fragment); - // Remove fragments that are larger than 50 % of the structure - if (size * 2 > inputStructure_.nAtoms()) + if (!neta.has_value()) { addInstance(instances.emplace_back(), fragmentAtoms); // Mask these fragment atoms for (const auto &unmasked : fragmentAtoms) atomMask.insert(unmasked); - - appendInstances(instances, detectedStructure, fragment); - - break; } - - for (const auto &fragmentAtom : fragmentAtoms) - { - if (atomMask.contains(fragmentAtom)) - continue; - - /* - * Best NETA definition - */ - - // Set up the return value and bind its contents - NETADefinition bestNETA; - std::vector rootAtoms; - - // Maintain a set of atoms matched by any NETA description we generate - std::set 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(fragmentAtom), std::nullopt, - Flags(NETADefinition::NETACreationFlags::ExplicitHydrogens, - NETADefinition::NETACreationFlags::IncludeRootElement)); - - // Apply this match over the whole fragment - std::vector currentRootAtoms; - for (auto fragmentAtom : fragmentAtoms) - { - 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; - } - - /* - * Get instances - */ - - // Get all atoms belonging to fragments from the same fragment size group - auto fragmentSizeGroupAtoms = getFragmentAtoms(inputStructure_, fragments); - - // Iterate over all structural atoms, matching their unit cell atoms by NETA - std::vector> matchedUnitCellAtomSets; - for (const auto &fragmentAtom : fragmentSizeGroupAtoms) + else + for (const auto &fragmentAtom : fragmentAtoms) { if (atomMask.contains(fragmentAtom)) continue; - auto matchedPath = neta.matchedPath(fragmentAtom).set(); - if (!matchedPath.empty()) - { - auto set = matchedUnitCellAtomSets.emplace_back(matchedPath); + // Get all atoms belonging to fragments from the same fragment size group + auto fragmentSizeGroupAtoms = getFragmentAtoms(fragments); - // Mask the current matched fragment atom - for (const auto &matchedAtom : set) - atomMask.insert(static_cast(matchedAtom)); + // Iterate over all structural atoms, matching their unit cell atoms by NETA + std::vector> matchedUnitCellAtomSets; + for (const auto &fragmentAtom : fragmentSizeGroupAtoms) + { + if (atomMask.contains(fragmentAtom)) + continue; + + auto matchedPath = neta->matchedPath(fragmentAtom).set(); + if (!matchedPath.empty()) + { + auto set = matchedUnitCellAtomSets.emplace_back(matchedPath); + + // Mask the current matched fragment atom + for (const auto &matchedAtom : set) + atomMask.insert(static_cast(matchedAtom)); + } } - } - // Loop over matched unit cell atoms, retrieving instances - for (const auto &matchedUnitCellAtoms : matchedUnitCellAtomSets) - { - if (matchedUnitCellAtoms.empty()) - continue; + // Loop over matched unit cell atoms, retrieving instances + for (const auto &matchedUnitCellAtoms : matchedUnitCellAtomSets) + { + if (matchedUnitCellAtoms.empty()) + continue; - addInstance(instances.emplace_back(), matchedUnitCellAtoms); + addInstance(instances.emplace_back(), matchedUnitCellAtoms); + } } - } - appendInstances(instances, detectedStructure, fragment); + if (!instances.empty()) + detectedStructures_.emplace_back(copyStructureAtomsAndBonds(detectedStructure, fragment)).instances() = + instances; } message("Detected {} distinct fragment structures:\n\n", detectedStructures_.size()); @@ -211,14 +141,13 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() */ // Copy atom and bond information from one structure to another -Structure &DetectMoleculesNode::copyStructureAtomsAndBonds(const Structure &source, Structure &target, - const std::vector fragmentAtomIndices) +Structure &DetectMoleculesNode::copyStructureAtomsAndBonds(Structure &target, const std::vector fragmentAtomIndices) const { // Copy fragment atoms, forming a map of the original indices to the new atom in the structure std::map originalIndexMap; for (auto fragAtomIndex : fragmentAtomIndices) { - const auto fragmentAtom = source.atom(fragAtomIndex); + const auto fragmentAtom = inputStructure_.atom(fragAtomIndex); originalIndexMap[fragAtomIndex] = target.addAtom(fragmentAtom->Z(), fragmentAtom->r(), fragmentAtom->q()); std::cout << std::format("New atom added to structure: {} {}\n", fragAtomIndex, Elements::symbol(fragmentAtom->Z())); } @@ -226,7 +155,7 @@ Structure &DetectMoleculesNode::copyStructureAtomsAndBonds(const Structure &sour // Copy bond information - since our fragment is by definition a bound fragment, we copy all bonds on each atom for (auto fragAtomIndex : fragmentAtomIndices) { - const auto fragmentAtom = source.atom(fragAtomIndex); + const auto fragmentAtom = inputStructure_.atom(fragAtomIndex); for (auto bond : fragmentAtom->bonds()) { // Add a bond between the new atoms in the detected structure (as long as it doesn't already exist) @@ -239,7 +168,7 @@ Structure &DetectMoleculesNode::copyStructureAtomsAndBonds(const Structure &sour } // Add fragment molecular instance -void DetectMoleculesNode::addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms) +void DetectMoleculesNode::addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms) const { std::visit( [&](const auto &atoms) @@ -251,47 +180,119 @@ void DetectMoleculesNode::addInstance(std::vector &targetInstance, cons } // Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments -std::vector DetectMoleculesNode::getFragmentAtoms(const Structure &structure, - const std::vector &fragmentIndices) +std::vector DetectMoleculesNode::getFragmentAtoms(const std::vector &fragmentIndices) const { std::vector fragmentAtoms; for (const auto &fragmentAtomIndex : fragmentIndices) - fragmentAtoms.push_back(structure.atom(int(fragmentAtomIndex))); + fragmentAtoms.push_back(inputStructure_.atom(int(fragmentAtomIndex))); return fragmentAtoms; } // Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments -std::vector DetectMoleculesNode::getFragmentAtoms(const Structure &structure, - const FragmentVector &fragmentIndices) +std::vector DetectMoleculesNode::getFragmentAtoms(const NETAFragmentVector &fragmentIndices) const { std::vector indices; std::size_t newSize = 0; for (const auto &v : fragmentIndices) ++newSize; indices.reserve(newSize); - for (const auto &v : fragmentIndices) + for (const auto &[_, v] : fragmentIndices) indices.insert(indices.end(), v.begin(), v.end()); - return getFragmentAtoms(structure, indices); + return getFragmentAtoms(indices); } // Find all molecular fragments -std::map DetectMoleculesNode::findMolecularFragments(const Structure &structure) +std::map DetectMoleculesNode::findMolecularFragments() const { - std::map map; + std::map map; - auto fragment = [structure](int i) { return Fragment>::get(structure.atoms(), i); }; + std::set alreadyInFragment; - for (int i = 0; i < structure.nAtoms(); i++) + for (int i = 0; i < inputStructure_.nAtoms(); i++) { - auto element = fragment(i); - const int size = element.size(); + auto fragmentIndices = Fragment>::get(inputStructure_.atoms(), i); + + // If any indices already within a fragment, continue + std::set fragmentIndicesSet(fragmentIndices.begin(), fragmentIndices.end()); + const int nNewIndices = fragmentIndicesSet.size(); + fragmentIndicesSet.merge(std::set(alreadyInFragment.begin(), alreadyInFragment.end())); + if (fragmentIndicesSet.size() != (alreadyInFragment.size() + nNewIndices)) + continue; + + // Register the current fragment indices + for (auto &idx : fragmentIndices) + alreadyInFragment.insert(idx); + + // Map fragment size to fragment indices + const int size = fragmentIndices.size(); if (!map.contains(size)) - map.emplace(size, FragmentVector{}); + map.emplace(size, NETAFragmentVector{}); + auto &targetFragments = map[size]; - targetFragments.push_back(element); + targetFragments.push_back( + {(size * 2 > inputStructure_.nAtoms()) ? std::optional{} : bestNETADefintion(fragmentIndices), + fragmentIndices}); } return map; -} \ No newline at end of file +} + +// Determine best NETA definition for index atoms within a fragment +NETADefinition DetectMoleculesNode::bestNETADefintion(const std::vector &fragmentIndices) const +{ + // Find the best NETA definition for this fragment + NETADefinition bestNETA; + std::vector rootAtoms; + + for (const auto &idx : fragmentIndices) + { + auto fragmentAtom = inputStructure_.atom(idx); + + // Maintain a set of atoms matched by any NETA description we generate + std::set 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(fragmentAtom), std::nullopt, + Flags(NETADefinition::NETACreationFlags::ExplicitHydrogens, + NETADefinition::NETACreationFlags::IncludeRootElement)); + + // Apply this match over the whole fragment + std::vector currentRootAtoms; + for (auto idx : fragmentIndices) + { + auto fragmentAtom = inputStructure_.atom(idx); + + 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; + } + } + + return bestNETA; +} diff --git a/src/nodes/detectMolecules.h b/src/nodes/detectMolecules.h index 095649c884..890f3f64e0 100644 --- a/src/nodes/detectMolecules.h +++ b/src/nodes/detectMolecules.h @@ -19,7 +19,7 @@ class Species; // DetectMolecules Node class DetectMoleculesNode : public Node { - using FragmentVector = std::vector>; + using NETAFragmentVector = std::vector, std::vector>>; using AtomCollection = std::variant, std::vector>; public: @@ -38,23 +38,24 @@ class DetectMoleculesNode : public Node Structure inputStructure_; // Output structures std::vector detectedStructures_; + // Lambda to determine if fragment is large enough to skip NETA creation + std::function largeFragment_{[this](int size) { return size * 2 > inputStructure_.nAtoms(); }}; /* * Helpers */ private: // Copy atom and bond information from one structure to another - static Structure ©StructureAtomsAndBonds(const Structure &source, Structure &target, - const std::vector fragmentIndices); + Structure ©StructureAtomsAndBonds(Structure &target, const std::vector fragmentIndices) const; // Add fragment molecular instance - static void addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms); + void addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms) const; // Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments - static std::vector getFragmentAtoms(const Structure &structure, - const std::vector &fragmentIndices); - static std::vector getFragmentAtoms(const Structure &structure, - const FragmentVector &fragmentIndices); + std::vector getFragmentAtoms(const std::vector &fragmentIndices) const; + std::vector getFragmentAtoms(const NETAFragmentVector &fragmentIndices) const; // Find all molecular fragments - static std::map findMolecularFragments(const Structure &structure); + std::map findMolecularFragments() const; + // Determine best NETA definition for index atoms within a fragment + NETADefinition bestNETADefintion(const std::vector &fragmentAtoms) const; /* * Processing diff --git a/src/nodes/insert.cpp b/src/nodes/insert.cpp index b3282e7580..f624607b56 100644 --- a/src/nodes/insert.cpp +++ b/src/nodes/insert.cpp @@ -19,8 +19,7 @@ InsertNode::InsertNode(Graph *parentGraph) : Node(parentGraph) addOutput("Configuration", "Modified configuration", configuration_); addInput("Population", "Population of the target to add", population_); addInput("Density", "Density at which to add the target", density_); - addInput("Species", "Source species or molecule set to add - all resulting molecules will have identical geometry", - speciesVariant_); + addInput("Species", "Source species or molecule set to add", speciesVariant_); addInput("Instances", "", instances_); // Options @@ -198,34 +197,10 @@ NodeConstants::ProcessResult InsertNode::process() break; } + // Prepare random number generation in case we are inserting via random sampling std::random_device rd; std::mt19937 gen(rd()); std::uniform_int_distribution<> distr(0, ipop - 1); - std::set alreadySampled; - - const auto sampleInstances = [&alreadySampled, &distr, &gen, ipop, this]() - { - const auto instances = this->instances_.instances(); - - int index = 0; - - // We've run out of new unsampled instances - if (alreadySampled.size() == instances.size()) - return -1; - - while (true) - { - auto sampledIndex = distr(gen); - if (alreadySampled.contains(sampledIndex)) - continue; - - index += sampledIndex; - alreadySampled.insert(index); - break; - } - - return index; - }; Matrix3 transform; const auto &box = configuration_->box(); @@ -242,20 +217,23 @@ NodeConstants::ProcessResult InsertNode::process() if (hasInstances) { std::vector atomicCoords; - if (instantiationMethod_ == InstantiationMethod::InstantiateAll) + switch (instantiationMethod_) { - atomicCoords = instances_.instances()[n]; - insertionComplete = true; - } - else - { - auto instancesIndex = sampleInstances(); - if (instancesIndex < 0) + case InstantiationMethod::InstantiateAll: + { + atomicCoords = instances_.instances()[n]; + insertionComplete = true; + break; + } + case InstantiationMethod::Sample: + atomicCoords = instances_.instances()[distr(gen)]; break; + default: + return error("Invalid instantiation method found (must be one of 'InstantialAll' or 'Sample')"); } // Update molecular atomic coordinates - for (int i = 0; i < mol->nAtoms(); i++) + for (auto i = 0; i < mol->nAtoms(); ++i) mol->atom(i)->setR(atomicCoords[i]); if (insertionComplete) diff --git a/src/nodes/parameter.h b/src/nodes/parameter.h index 1986b9b566..2fb57ca779 100644 --- a/src/nodes/parameter.h +++ b/src/nodes/parameter.h @@ -52,7 +52,6 @@ class ParameterBase : public Serialisable ClearData, /* Indicates that any local data should be cleared if the parameter is changed */ Input, /* Indicates that the parameter is meant to be a sink for data and not a source */ Output, /* Indicates that the parameter is meant to be a source of data and not a sink */ - Dynamic, /* Indicates that the parameter is meant to be a dynamic source of data and not a sink */ }; // Allowed Edge Count enum AllowedEdgeCount diff --git a/tests/nodes/parameters.cpp b/tests/nodes/parameters.cpp index 95165b3634..7e749ce9ee 100644 --- a/tests/nodes/parameters.cpp +++ b/tests/nodes/parameters.cpp @@ -362,21 +362,21 @@ TEST_F(ParametersTest, OptionalPointerToVariant) TEST_F(ParametersTest, DynamicOutput) { ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "Sender")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverA")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverB")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverC")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverD")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverE")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverA")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverB")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverC")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverD")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverE")); auto sender = testGraph_.findNode("Sender"); ASSERT_TRUE(sender->setInput("Message", std::string("hello"))); ASSERT_EQ(testGraph_.runDynamic(sender, { - {"Sender", "Message-Part-0", "RecieverA", "Char"}, - {"Sender", "Message-Part-1", "RecieverB", "Char"}, - {"Sender", "Message-Part-2", "RecieverC", "Char"}, - {"Sender", "Message-Part-3", "RecieverD", "Char"}, - {"Sender", "Message-Part-4", "RecieverE", "Char"}, + {"Sender", "Message-Part-0", "ReceiverA", "Char"}, + {"Sender", "Message-Part-1", "ReceiverB", "Char"}, + {"Sender", "Message-Part-2", "ReceiverC", "Char"}, + {"Sender", "Message-Part-3", "ReceiverD", "Char"}, + {"Sender", "Message-Part-4", "ReceiverE", "Char"}, }), NodeConstants::ProcessResult::Success); @@ -384,7 +384,7 @@ TEST_F(ParametersTest, DynamicOutput) std::vector chars; for (const auto &which : {"A", "B", "C", "D", "E"}) { - auto node = testGraph_.findNode("Reciever" + std::string(which)); + auto node = testGraph_.findNode("Receiver" + std::string(which)); chars.push_back(node->findInput("Char")->get()); } @@ -395,21 +395,21 @@ TEST_F(ParametersTest, DynamicOutput) TEST_F(ParametersTest, DynamicPointerOutput) { ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "Sender")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverA")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverB")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverC")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverD")); - ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "RecieverE")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverA")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverB")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverC")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverD")); + ASSERT_TRUE(testGraph_.addNode(std::make_unique(&testGraph_), "ReceiverE")); auto sender = testGraph_.findNode("Sender"); ASSERT_TRUE(sender->setInput("Message", std::string("hello"))); ASSERT_EQ(testGraph_.runDynamic(sender, { - {"Sender", "Message-Ptr-Part-0", "RecieverA", "CharPtr"}, - {"Sender", "Message-Ptr-Part-1", "RecieverB", "CharPtr"}, - {"Sender", "Message-Ptr-Part-2", "RecieverC", "CharPtr"}, - {"Sender", "Message-Ptr-Part-3", "RecieverD", "CharPtr"}, - {"Sender", "Message-Ptr-Part-4", "RecieverE", "CharPtr"}, + {"Sender", "Message-Ptr-Part-0", "ReceiverA", "CharPtr"}, + {"Sender", "Message-Ptr-Part-1", "ReceiverB", "CharPtr"}, + {"Sender", "Message-Ptr-Part-2", "ReceiverC", "CharPtr"}, + {"Sender", "Message-Ptr-Part-3", "ReceiverD", "CharPtr"}, + {"Sender", "Message-Ptr-Part-4", "ReceiverE", "CharPtr"}, }), NodeConstants::ProcessResult::Success); @@ -417,7 +417,7 @@ TEST_F(ParametersTest, DynamicPointerOutput) std::vector chars; for (const auto &which : {"A", "B", "C", "D", "E"}) { - auto node = testGraph_.findNode("Reciever" + std::string(which)); + auto node = testGraph_.findNode("Receiver" + std::string(which)); chars.push_back(*node->findInput("CharPtr")->get()); } From be37962eb695bfd3af32f8269f856c61b7923b4a Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Tue, 14 Jul 2026 11:13:19 +0100 Subject: [PATCH 7/9] streamline a bit - don't make StructureAtom * vectors from indices, just call structure.atom(i).. --- src/nodes/detectMolecules.cpp | 33 ++++++++++++--------------------- src/nodes/detectMolecules.h | 5 ++--- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/nodes/detectMolecules.cpp b/src/nodes/detectMolecules.cpp index b9f5057b44..6c2339d500 100644 --- a/src/nodes/detectMolecules.cpp +++ b/src/nodes/detectMolecules.cpp @@ -55,21 +55,21 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() detectedStructure.createBox(inputStructure_.box().axes()); std::vector> instances; - // Get fragment atoms - auto fragmentAtoms = getFragmentAtoms(fragment); - if (!neta.has_value()) { - addInstance(instances.emplace_back(), fragmentAtoms); + // Add instances + auto &instanceAtoms = instances.emplace_back(); + for (auto &fragmentAtomIndex : fragment) + instanceAtoms.push_back(inputStructure_.atom(fragmentAtomIndex)->r()); // Mask these fragment atoms - for (const auto &unmasked : fragmentAtoms) - atomMask.insert(unmasked); + for (const auto &unmaskedAtomIndex : fragment) + atomMask.insert(inputStructure_.atom(unmaskedAtomIndex)); } else - for (const auto &fragmentAtom : fragmentAtoms) + for (const auto &fragmentAtomIndex : fragment) { - if (atomMask.contains(fragmentAtom)) + if (atomMask.contains(inputStructure_.atom(fragmentAtomIndex))) continue; // Get all atoms belonging to fragments from the same fragment size group @@ -99,7 +99,10 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() if (matchedUnitCellAtoms.empty()) continue; - addInstance(instances.emplace_back(), matchedUnitCellAtoms); + // Add instances + auto &instanceAtoms = instances.emplace_back(); + for (const auto &matchedUnitCellAtom : matchedUnitCellAtoms) + instanceAtoms.push_back(matchedUnitCellAtom->r()); } } @@ -167,18 +170,6 @@ Structure &DetectMoleculesNode::copyStructureAtomsAndBonds(Structure &target, co return target; } -// Add fragment molecular instance -void DetectMoleculesNode::addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms) const -{ - std::visit( - [&](const auto &atoms) - { - for (const AtomBase *atom : atoms) - targetInstance.push_back(atom->r()); - }, - instanceFragmentAtoms); -} - // Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments std::vector DetectMoleculesNode::getFragmentAtoms(const std::vector &fragmentIndices) const { diff --git a/src/nodes/detectMolecules.h b/src/nodes/detectMolecules.h index 890f3f64e0..d32f7158d8 100644 --- a/src/nodes/detectMolecules.h +++ b/src/nodes/detectMolecules.h @@ -19,7 +19,8 @@ class Species; // DetectMolecules Node class DetectMoleculesNode : public Node { - using NETAFragmentVector = std::vector, std::vector>>; + using FragmentIndices = std::vector; + using NETAFragmentVector = std::vector, FragmentIndices>>; using AtomCollection = std::variant, std::vector>; public: @@ -47,8 +48,6 @@ class DetectMoleculesNode : public Node private: // Copy atom and bond information from one structure to another Structure ©StructureAtomsAndBonds(Structure &target, const std::vector fragmentIndices) const; - // Add fragment molecular instance - void addInstance(std::vector &targetInstance, const AtomCollection &instanceFragmentAtoms) const; // Get fragment atoms from either a single set of fragment indices, or in its overloaded form, a vector of fragments std::vector getFragmentAtoms(const std::vector &fragmentIndices) const; std::vector getFragmentAtoms(const NETAFragmentVector &fragmentIndices) const; From 7e222122f13fb9304130745765a9fc27d0808f8b Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Tue, 14 Jul 2026 11:42:19 +0100 Subject: [PATCH 8/9] streamline a bit - don't make StructureAtom * vectors from indices, just call structure.atom(i).. (part 2) --- src/nodes/detectMolecules.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/nodes/detectMolecules.h b/src/nodes/detectMolecules.h index d32f7158d8..80111f4abe 100644 --- a/src/nodes/detectMolecules.h +++ b/src/nodes/detectMolecules.h @@ -21,7 +21,6 @@ class DetectMoleculesNode : public Node { using FragmentIndices = std::vector; using NETAFragmentVector = std::vector, FragmentIndices>>; - using AtomCollection = std::variant, std::vector>; public: DetectMoleculesNode(Graph *parentGraph); From 8c9e7c2eb35dcc13b745e27b4468179753941d35 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Wed, 15 Jul 2026 10:28:31 +0100 Subject: [PATCH 9/9] use an erase/remove_if idiom --- src/nodes/detectMolecules.cpp | 83 +++++++++++++++-------------------- 1 file changed, 36 insertions(+), 47 deletions(-) diff --git a/src/nodes/detectMolecules.cpp b/src/nodes/detectMolecules.cpp index 6c2339d500..01211b125c 100644 --- a/src/nodes/detectMolecules.cpp +++ b/src/nodes/detectMolecules.cpp @@ -45,11 +45,12 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() "Can't create molecular definitions since this unit cell appears to be a continuous framework/network. Consider " "adjusting the bonding options in order to generate molecular fragments.\n"); - std::set atomMask; - - for (const auto &[size, fragments] : fragmentMap) - for (const auto &[neta, fragment] : fragments) + for (auto &[_, fragments] : fragmentMap) + for (auto &[neta, fragment] : fragments) { + // Make a const copy of the current fragment for later reference + const auto currentFragment = fragment; + // Create a provisional structure for the detected fragment Structure detectedStructure; detectedStructure.createBox(inputStructure_.box().axes()); @@ -59,55 +60,43 @@ NodeConstants::ProcessResult DetectMoleculesNode::process() { // Add instances auto &instanceAtoms = instances.emplace_back(); - for (auto &fragmentAtomIndex : fragment) + for (auto &fragmentAtomIndex : currentFragment) instanceAtoms.push_back(inputStructure_.atom(fragmentAtomIndex)->r()); - // Mask these fragment atoms - for (const auto &unmaskedAtomIndex : fragment) - atomMask.insert(inputStructure_.atom(unmaskedAtomIndex)); + break; } else - for (const auto &fragmentAtomIndex : fragment) - { - if (atomMask.contains(inputStructure_.atom(fragmentAtomIndex))) - continue; - - // Get all atoms belonging to fragments from the same fragment size group - auto fragmentSizeGroupAtoms = getFragmentAtoms(fragments); - - // Iterate over all structural atoms, matching their unit cell atoms by NETA - std::vector> matchedUnitCellAtomSets; - for (const auto &fragmentAtom : fragmentSizeGroupAtoms) - { - if (atomMask.contains(fragmentAtom)) - continue; - - auto matchedPath = neta->matchedPath(fragmentAtom).set(); - if (!matchedPath.empty()) - { - auto set = matchedUnitCellAtomSets.emplace_back(matchedPath); - - // Mask the current matched fragment atom - for (const auto &matchedAtom : set) - atomMask.insert(static_cast(matchedAtom)); - } - } - - // Loop over matched unit cell atoms, retrieving instances - for (const auto &matchedUnitCellAtoms : matchedUnitCellAtomSets) - { - if (matchedUnitCellAtoms.empty()) - continue; - - // Add instances - auto &instanceAtoms = instances.emplace_back(); - for (const auto &matchedUnitCellAtom : matchedUnitCellAtoms) - instanceAtoms.push_back(matchedUnitCellAtom->r()); - } - } + { + // Iterate over all structural atoms, matching their unit cell atoms by NETA + fragments.erase( + std::remove_if(fragments.begin(), fragments.end(), + [this, &instances, &neta, ¤tFragment](auto &pair) -> bool + { + auto &[_, fragmentAtomIndices] = pair; + + for (const auto &fragmentAtomIndex : fragmentAtomIndices) + { + auto matchedUnitCellAtoms = + neta->matchedPath(this->inputStructure_.atom(fragmentAtomIndex)).set(); + + if (!matchedUnitCellAtoms.empty()) + { + // Add instances + auto &instanceAtoms = instances.emplace_back(); + for (const auto &matchedUnitCellAtom : matchedUnitCellAtoms) + instanceAtoms.push_back(matchedUnitCellAtom->r()); + + return true; + } + } + + return false; + }), + fragments.end()); + } if (!instances.empty()) - detectedStructures_.emplace_back(copyStructureAtomsAndBonds(detectedStructure, fragment)).instances() = + detectedStructures_.emplace_back(copyStructureAtomsAndBonds(detectedStructure, currentFragment)).instances() = instances; }