Addition of the MOFs core M6L8L12 #529
Conversation
|
Hey @iribirii , can you give some context for this topology graph? A paper with this structure, for example? |
|
Sure @andrewtarzia ! Figure 2 in this paper show the structure of the core clearly |
|
Ahh, it's the metal node of a mof, I see now. Although, I can definitely see how it can be a cage too! I think this makes sense. |
|
Yeah I wasn't sure where to put it and, since I used the M6L12 cage as a template, it felt adequate to leave it under the same category. But maybe it could be a sub-family within the MOF and OF families. Up to you 😄 |
|
I think this makes sense! Ultimately, there is no real chemistry limiting it's use elsewhere! |
There was a problem hiding this comment.
Apologies for the delay -- this looks really good! Just one thing, could you add a test structure into https://github.com/lukasturcani/stk/tree/master/tests/molecular/molecules/molecule/fixtures/cage/metal_topologies like the other topologies, you will also need to update the fixutre in this file so that your test runs https://github.com/lukasturcani/stk/blob/master/tests/molecular/molecules/molecule/fixtures/cage/cage.py
Once that's done the test will generate a .npy file which you will also need to commit. This ensures that stk does not randomly change the geometry of structures it builds.
| M6L8L12 Cuboctahedron | ||
| ========== | ||
|
|
||
| """ |
There was a problem hiding this comment.
no need to add docstrings to internal modules
There was a problem hiding this comment.
I added the tests to both files above, however when I run the tests locally I get an 'CallSpec2' object has no attribute 'funcargs' message... I guess it might be a problem of the pytest version I have installed?
I am not sure, it is the first time I have to run a real test 🤣
There was a problem hiding this comment.
Ahh yes, this issue should be solved in the latest version of stk by limiting the pytest version to < 8 (see the pyproject.toml file). You may need to reinstall your development version with just dev to get the tests working.
|
@iribirii -- don't worry about the failing |
| bb1:range(6,18), | ||
| bb2:range(18,26) | ||
| }, | ||
| optimizer=stk.Nulloptimizer(), |
| @@ -41,6 +41,7 @@ | |||
| lazy_fixture("metal_cage_m4l8"), | |||
| lazy_fixture("metal_cage_m6l2l3_prism"), | |||
| lazy_fixture("metal_cage_m6l12_cube"), | |||
| lazy_fixture("metal_cage_m6l812_cuboctahedron"), | |||
There was a problem hiding this comment.
typo: missing l between 8 and 12
|
To fix the remaining tests:
|
|
Ah and anothert thing -- can you add the structure to https://github.com/lukasturcani/stk/blob/master/docs/source/cage.rst -- so that it appears in the docs? 🙏 |
|
And you need to add an import to this file too https://github.com/lukasturcani/stk/blob/master/tests/molecular/molecules/molecule/fixtures/cage/metal_topologies/__init__.py -- so taht the tests are able to find the fixture |
|
and the delete the docstring for the internal module here https://github.com/lukasturcani/stk/pull/529/files#r1598731170 |
|
it looks like your test case is broken -- you dont have to re-use |
| import moldoc.molecule as molecule | ||
| import stk | ||
|
|
||
| m1 = stk.BuildingBlock('[Ti](Br)(Br)(Br)(Br)(Br)(Br)(Br)(Br)', functional_groups=[stk.BromoFactory()]) |
There was a problem hiding this comment.
Can you change all line lengths to be less than 72, even if in doc string?
|
@iribirii fwiw -- the test case is just an example molecule constructed with your new topology graph, and the smiles is the structure you expect to make. this ensures that the topology graph is making what you expect it to make and that stk keeps making the same things acros updates -- lemme know if anything is unclear! |
|
Hi @iribirii -- are you still interested in finishing work on the PR? |
Requested Reviewers: @lukasturcani
Note for Reviewers: If you accept the review request add a 👍 to this post