-
Notifications
You must be signed in to change notification settings - Fork 19
feat: Species node #2271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Species node #2271
Conversation
b8c0726 to
911c9ab
Compare
911c9ab to
d00006e
Compare
| target[tag] = {{"index", userIndex()}, {"z", Z_}, {"r", r_}, {"charge", charge_}, {"type", atomType_->name().data()}}; | ||
| target[tag] = {{"index", userIndex()}, {"z", Z_}, {"r", r_}, {"charge", charge_}}; | ||
| if (atomType_) | ||
| target[tag]["type"] = atomType_->name().data(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we expect problems if atomType_ is null? It seems like in that case, the "type" key-value pair of target[tag] will be missing, which we may well need to catch somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The atomType_ is optional in the SpeciesAtom and line 499 (in the deserialise) method handles with case by checking for the "type" tag with optionalOn. That way, if the "type" tag is missing, the atomType_ is null.
| text: "Phantom" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this may be and ideal case for a QAbstractItemModel to provide a reusable backend PeriodicTable object. It could then hold element specific attributes, such as atomic number and maybe even incorporate the datasets related to scattering length density or isotope weights. In some cases, it could be used to provide a simple drop-down like the above, but with a single line of code i.e.
model: periodicTable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I love that idea for my next PR. It also provides a very basic answer to my concern about whether to use the element name or symbol: each can be their own role and we could even allow the user to decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of sense from the perspective of the UI, but the PeriodicTable class being proposed (which sounds like a good idea in principle) cannot have any dependence on Qt if it is to be used / accessed in the CLI code (which it frequently is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is mainly about creating a wrapper model around the Elements namespace that can be accessed from the GUI. None of the CLI code would be touched, but the model could read the Elements to populate its own data before sending it to the GUI, instead of having the ElementComboBox.qml contain a duplicate copy.
Anything that might cause changes to the CLI (e.g. adding SLD information) would be done by adding it into the Elements namespace and then the GUI model simply updated to access that information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this I'm wondering if there is value in resurrecting my generalised table model PR (#1959)...
| SpeciesNode::SpeciesNode(Graph *parentGraph, std::shared_ptr<Species> &&species) | ||
| : Node(parentGraph), species_(std::move(species)) | ||
| { | ||
| addOption<std::shared_ptr<Species>>("Species", "Created Species", species_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this option is necessary?
|
Actually - one big comment: For |
The primary goal of this PR is to add a Species node to the graph system. This is mostly contained in
src/nodes/species.handsrc/gui/models/speciesModel.h. However, the following effects ensure:DissolveModel. Therefore, theSpeciesModelclass was modified from modeling a vector of species to (more closely) modeling a single species.The following improvements are outside the scope of the current PR:
Open Questions: