Refactor CIF Import#1607
Conversation
trisyoungs
left a comment
trisyoungs
left a comment
There was a problem hiding this comment.
First off, I really like the way this cleans up the wizard code and collects all of the CIF stuff together - clear separation, like it a lot.
I don't think CIFSpecies is an appropriate class name any longer since it does an awful lot more than that. As a related broad comment I wonder about whether the functionality of CIFSpecies should be absorbed into CIFImporter, and that class renamed to something like CIFHandler? Having the import functionality as a separate thing doesn't offer a lot.
My final comment relates to unit testing, which is now one thing we'll be able to do once this refactor is complete. I think it would be most wise to include the testing in this PR - I'm happy to work on that once the present changes have settled.
trisyoungs
force-pushed
the
1576-refactor-cif-import
branch
2 times, most recently
from
dabe64c to
2c4053e
Compare
jws-1
left a comment
jws-1
left a comment
There was a problem hiding this comment.
I like where things are going - I'm quite pleased with our new interface!
trisyoungs
force-pushed
the
1576-refactor-cif-import
branch
2 times, most recently
from
e76725d to
08b004a
Compare
…ls of the cell partitioning.
trisyoungs
force-pushed
the
1576-refactor-cif-import
branch
from
08b004a to
6948ea7
Compare
trisyoungs
force-pushed
the
1576-refactor-cif-import
branch
from
6948ea7 to
edbbcbb
Compare
rprospero
left a comment
rprospero
left a comment
There was a problem hiding this comment.
This came together beautifully. The only comments I have are praise.
| // CIF Species Update Flags | ||
| enum UpdateFlags | ||
| { | ||
| CleanMoietyRemoveAtomics, /* Remove atoms of single moiety */ | ||
| CleanMoietyRemoveWater, /* Remove water molecules of single moiety */ | ||
| CleanMoietyRemoveNETA, /* Remove single atoms by NETA definition */ | ||
| CleanRemoveBoundFragments, /* Remove entire fragments when using NETA definition */ | ||
| CalculateBonding, /* Calculate bonding */ | ||
| PreventMetallicBonding, /* Prevent metallic bonding */ | ||
| CreateSupermolecule | ||
| }; |
There was a problem hiding this comment.
This flag enum makes me unreasonably happy
| std::vector<int> allIndices(sp->nAtoms()); | ||
| std::iota(allIndices.begin(), allIndices.end(), 0); | ||
| std::vector<int> indicesToRemove; | ||
| std::set_difference(allIndices.begin(), allIndices.end(), fragment.begin(), fragment.end(), |
There was a problem hiding this comment.
I'd never seen set_difference before, but it's the perfect tool for the job.
|
Also happy with this! |
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
3 participants
This PR refactors the CIF importing, moving the bulk of the code from the dialog into
cifClasses. A single class has been addedCIFSpeciesthat now does the work.I think this is moving in the right direction, but further work likely needs to be done - so a second pair of eyes would be useful!
Works towards #1576.