Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add bonds for structure type entries #465

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Add bonds for structure type entries #465

Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cbe0641
Add `bonds` for structure type entries.
merkys Jun 6, 2023
1478145
Reformulate the meaning of `null`.
merkys Jun 7, 2023
9502864
Update optimade.rst
merkys Jun 7, 2023
5460c5a
Update optimade.rst
merkys Jun 7, 2023
a49da9e
Addressing the ordering of site indexes.
merkys Jun 7, 2023
c016d01
Merge branch 'connectivity' of github.com:merkys/OPTIMADE into connec…
merkys Jun 7, 2023
7bf9f2b
Adding a missing empty line.
merkys Jun 7, 2023
a448690
Add means to define translation vectors.
merkys Jun 7, 2023
6bfa206
Update optimade.rst
merkys Jun 7, 2023
e4b870c
Sites are 0-based integers.
merkys Jun 8, 2023
8a37350
Merge branch 'connectivity' of github.com:merkys/OPTIMADE into connec…
merkys Jun 8, 2023
d0ab465
Adjust example description.
merkys Jun 8, 2023
0b0326a
Update optimade.rst
merkys Jun 8, 2023
b682c70
Slightly modifying the addition from @ml-evs
merkys Jun 8, 2023
7c1102b
Remove trailing whitespace.
merkys Jun 8, 2023
e4ac0cc
Update optimade.rst
merkys Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions optimade.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2833,6 +2833,35 @@ assemblies
These two sites are correlated (either site 2 or 3 is present).
However, the presence or absence of sites 0 and 1 is not correlated with the presence or absence of sites 2 and 3 (in the specific example, the pair of sites (0, 2) can occur with 0.2\*0.3 = 6 % probability; the pair (0, 3) with 0.2\*0.7 = 14 % probability; the pair (1, 2) with 0.8\*0.3 = 24 % probability; and the pair (1, 3) with 0.8\*0.7 = 56 % probability).

bonds
~~~~~

- **Description**: A list describing the chemical connectivity of the structure.
- **Type**: list of dictionary with keys:

- :property:`sites`: a list of integers (REQUIRED)
- :property:`translations`: a list of list of integers (OPTIONAL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably specify very clearly that translations must be applied to the coordinates of atoms as they are currently given, and not to, e.g., coordinates reduced to the [0;1) unit cell.


- **Requirements/Conventions**:

- **Support**: OPTIONAL support in implementations, i.e., MAY be :val:`null`.
- **Query**: Support for queries on this property is OPTIONAL.
If supported, filters MAY support only a subset of comparison operators.
- The property SHOULD be :val:`null` for structures for which the chemical connectivity is unknown to the implementation.
- If present, it MUST be a list of dictionaries, each of which represents a chemical bond and MUST have the following key:

- *sites*: a non-decreasing list of 0-based indexes of the two sites that form a chemical bond.

- If translations are needed by at least one of the sites of a bond, the following key SHOULD be used:
Copy link
Contributor

@vaitkus vaitkus Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the translations field is not mandatory in general, maybe we should require it if at least one of the sites is translated?

That is, maybe we should change SHOULD to MUST?

Edit: changed the field name from sites to translations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you are talking about translations, as sites is mandatory?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merkys Yes, I actually meant translations (fixed).


- *translations*: a list of two lists of three integers each, defining translations of the sites.
Omitting this key means that both translation vectors are :val:`[0, 0, 0]`.

merkys marked this conversation as resolved.
Show resolved Hide resolved
- **Examples**:

- :val:`[ {"sites": [1, 2]} ]`: a structure with a bond between sites 1 and 2.
vaitkus marked this conversation as resolved.
Show resolved Hide resolved
- :val:`[ {"sites": [1, 1], "translations": [ [0, 0, 0], [0, 0, 1] ]} ]`: a 1D polymer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current scheme, I think this is the representation of example of primitive NaCl:

Suggested change
- :val:`[ {"sites": [1, 1], "translations": [ [0, 0, 0], [0, 0, 1] ]} ]`: a 1D polymer.
- :val:```
[ {"sites": [0, 1], "translations": [ [0, 0, 0], [-1, -1, 0] ]},
{"sites": [0, 1], "translations": [ [0, 0, 0], [-1, 0, -1] ]},
{"sites": [0, 1], "translations": [ [0, 0, 0], [0, -1, -1] ]},
{"sites": [0, 1], "translations": [ [0, 0, 0], [1, 1, 0] ]}
{"sites": [0, 1], "translations": [ [0, 0, 0], [1, 0, 1] ]}
{"sites": [0, 1], "translations": [ [0, 0, 0], [0, 1, 1] ]}
]
```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property could potentially be used to describe crystal topology which may involve various types of contact distances, but that seems to be a bit a beast of its own (at least according to work done in the TOPO_CIF dictionary [1]). I would probably limit it to chemical bonding for now, especially since currently we purposely are trying to avoid specifying the bond type/order.

[1] https://github.com/COMCIFS/TopoCif/blob/main/dictionary/Topology_0.9.5.dic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaitkus Right, there have already been discussions in the workshop about having different networks for the same structure entry, but this seems to be difficult to accommodate at the moment.


structure\_features
~~~~~~~~~~~~~~~~~~~

Expand Down