refactor: Instances in species - #2598
Conversation
…lication to unit tests.
trisyoungs
changed the title
RobBuchananCompPhys
left a comment
RobBuchananCompPhys
left a comment
There was a problem hiding this comment.
Looks very good 👍 , particularly like the clean logic flow in ReplicatedConfigurationNode. Would be good to get your thoughts on a couple of suggestions!
| addOutput(name, "Detected molecular structure", structure); | ||
| else | ||
| { | ||
| // TODO Need to update the underlying ParameterBase here, but how? |
There was a problem hiding this comment.
Is this outlying task going to be a blocker on the proper functioning of the node? Why/in what way exactly do we need to update the ParameterBase?
There was a problem hiding this comment.
So this was a realisation that I had - the ParameterBase stores a reference to the Structure, but if the DetectMolecules node runs again the Structures are all recreated so the reference won't be valid any longer. We can't just replace the reference with another one, so we would have to recreate the dynamic output which means losing any connectivity we had to it before. Maybe this isn't a big issue in practice because - we need the UI to find out I guess!
|
|
||
| replicated_.updateObjectRelationships(); | ||
|
|
||
| message("Created {}x{}x{} replica - {} atoms total.\n", repeat_.x, repeat_.y, repeat_.z, replicated_.nAtoms()); |
There was a problem hiding this comment.
| message("Created {}x{}x{} replica - {} atoms total.\n", repeat_.x, repeat_.y, repeat_.z, replicated_.nAtoms()); | |
| message("Configuration replicated: New dimensions are {}x{}x{}, {} atoms total.\n", repeat_.x, repeat_.y, repeat_.z, replicated_.nAtoms()); |
Really pedantic suggestion here (!). Although replication sounds fine as a description of the process taking place, the idea of a "replica" doesn't quite sit right (sounds a little too much like "made an exact copy"!). I suggest we frame this message slightly differently.
There was a problem hiding this comment.
OK, what about "Configuration contents replicated ({}x{}x{}) - {} atoms in new configuration."?
2 participants
This PR adds instance storage to the
Speciesclass, introduces theInstantiateNodefor adding detected molecule replicas into a configuration, and performs some refactoring on the CIF unit tests.