refactor: AverageMolecule node#2490
Conversation
rprospero
left a comment
rprospero
left a comment
There was a problem hiding this comment.
I have a question/suggestion about how we're handling the sampled coordinates, but everything else looks good
| if (!sampledX_) | ||
| { | ||
| message("Initialising arrays for average molecule: size = {}\n", requiredSize); | ||
|
|
||
| sampledX_.emplace().initialise(requiredSize); | ||
| sampledY_.emplace().initialise(requiredSize); | ||
| sampledZ_.emplace().initialise(requiredSize); | ||
|
|
||
| // Copy current geometry of species to set up atom / bond relationships in the Structure | ||
| structure_ = targetSpecies->asStructure(); | ||
| } |
There was a problem hiding this comment.
If I change the target species in the GUI, does sampledX_ get resized to the new required size? The current logic seems to only check if the vector has every been initialised at all.
I'm wondering if we might be better off using the raw vector instead of an optional one. Our test would then be whether the vector has the correct length, instead of checking whether the optional has been filled.
There was a problem hiding this comment.
So this is effectively captured by issue #2488 which I put in yesterday - changing the input Site (== Species) should absolutely force the data to be cleared and reset. We should handle this in a general way at the Node/Parameter level, and we already have virtual Node::clearData(). What we don't currently have is the flag on Parameter to call that function if the parameter data changes, hence #2488.
2 participants
Heavy refactor of
AvgMolModuleintoAverageMoleculeNode.Obsoletes #2328.