Handle periodic cell optimization with stk.Optimizer classes #282
Comments
Merged
lukasturcani
added a commit
that referenced
this issue
Apr 26, 2021
Issues addressed: #282, #278 This commit adds the following features: * A new `Optimizer`, `PeriodicCollapser`, which can be used with periodic COF topology graphs. * A subclass of `ConstructionResult`, called `PeriodicConstructionResult`, which additionally holds periodic information about the constructed molecule. * A new initializer for `ConstructedMolecule`, `init_from_construction_result`. This new initializer can be used when the user calls `TopologyGraph.construct()` directly, and means that they can get the `ConstructionResult` and the corresponding `ConstructedMolecule`, without having to run construction twice. This commit also deprecates the method `TopologyGraph.get_periodic_info()`, use `PeriodicConstructionResult.get_periodic_info()` instead. There were a couple of changes which needed to be made to add the `PeriodicCollapser`. The main issue was that the `PeriodicCollapser` needs to be able to modify the lattice constants, so that the `PeriodicInfo` returned by `PeriodicConstructionResult.get_periodic_info()`, has the lattice constants shrunk by the same amount the `ConstructedMolecule` is. In order to do this, the `ConstructionState` and `GraphState` were extended to allow modification of the lattice constants via `with_lattice_constants()` and retrieval of the lattice constants via `get_lattice_constants()`. The `PeriodicInfo` is now being returned by the `PeriodicConstructionResult` rather than by the `TopologyGraph` because it avoids needing to run the construction twice, if the user wishes to get the `ConstructedMolecule` and the `PeriodicInfo`. If the `PeriodicInfo` was returned by the `TopologyGraph`, `TopologyGraph.get_periodic_info()` would have to run a construction. This is because it is only after the construction that the value by which the `PeriodicInfo` should be shrunk by is known. If the user wishes to get both the `PeriodicInfo` and the `ConstructedMolecule` in one go, they can call `TopologyGraph.construct()` explicitly. There is an important design concept this commit highlighted. The return value of `TopologyGraph.construct` returns the `ConstructionResult` which holds the data necessary to create a `ConstructedMolecule`, but also additional data which does not belong on the `ConstructedMolecule`, `PeriodicInfo` is an example of that. `ConstructionResult` can be subclassed, as in `PeriodicConstructionResult`, if the additional data is specific to some subset of `TopologyGraph`classes. The importance of having both `ConstructedMolecule` and `ConstructionResult` is that you can do very general things with a `ConstructedMolecule`, such as serialization, which you cannot do with a `ConstructionResult`. However, `ConstructionResult` may hold specific data which you cannot find on a `ConstructedMolecule`. So one is generic while the other is specific, and this means each has different use cases. By allowing the conversion from `ConstructionResult` to `ConstructedMolecule`, the user can use whatever fits their use case. The `ConstructionState`, `ConstructionResult` and `GraphState` classes have the method `get_num_building_block()` and `get_building_blocks()` added. This was necessary so that `ConstructedMolecule.init_from_construction_result()` can be initialized from `ConstructionResult` only, rather than both `ConstructionResult` and `TopologyGraph`. The `ConstructedMolecule` needs to create the `_num_building_blocks` attribute, for which it needs to call `get_num_building_block()` and `get_building_blocks()`. Previously these were found on the `TopologyGraph` only, but now I'm moving them into the `ConstructionResult` too. Having to initialize from both a `ConstructionResult` and `TopologyGraph` is bad because there is no guarantee that the `ConstructionResult` was produced by the `TopologyGraph` you pass in as the argument. I considered replacing `get_building_blocks()` and `get_num_building_block()`, with a single method, `get_num_building_blocks()`, which would return a `dict` which maps each building block to the number of times it appears in the `ConstructedMolecule`. However, I decided against this because the API `get_building_block()` and `get_num_building_block` already exists on `TopologyGraph` and `ConstructedMolecule`, so this keeps the APIs of `ConstructionState`, `ConstructionResult` and `GraphState` consistent. In addition, the order of building blocks is important, and while `dict` instances are ordered, having a `get_building_blocks()` method makes the importance of ordering more clear, and easier to document, in my opinion. Co-authored-by: Andrew Tarzia <andrew.tarzia@gmail.com>
|
Fixed by #313 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, the periodic info will not be updated along with the structure during optimization. The MCHammer module does not handle unit cell optimisation.
The text was updated successfully, but these errors were encountered: