search

OpenChemistry / chemicaljson

Development of the Chemical JSON data representation
63 stars 15 forks source link

Connections reflexive? #6

Closed mcocdawc closed 6 years ago

mcocdawc commented 6 years ago

Thank you for this project! Could you please specify the reflexivity of the connections object. So if atom 1 is bonded to atom 0, then atom 0 is bonded to atom 1. In your minimal example it is non reflexive.

I think there are three options:

  1. Non reflexive
  2. Reflexive
  3. Could be both and code may not rely on one property
cryos commented 6 years ago

You might have to clarify what you mean by reflexive, but bonds are two way, so we won't specify a bond between atom 0 and 1, and another bond between atom 1 and 0. One is equivalent to the other, ideally the first index is the lower number, but that is not guaranteed nor tested.

mcocdawc commented 6 years ago

My wording was indeed a bit weird. To say it in another way. If I create a cjson file in my library, I have to assert that I don't specify bonds twice. So the list has to be:

[0, 1, 1, 2...]
# instead of
[0, 1, 1, 0, 1, 2, 2, 1...]

My question was simply if both options are valid or if one is the only correct one.

cryos commented 6 years ago

The first option is valid, the second may well result in the bonds being rendered twice, and possibly other odd behavior. It is something I hadn't thought about a lot, and I am not sure why you would want to specify both, our APIs usually look at whether there is a bond between two atoms, it is generally expected to be one bond, with a bond order, and possibly other properties attached. Bonds aren't directional, at least not in the work I have done, and so specifying the reverse would usually be removed as a duplicate.

mcocdawc commented 6 years ago

I stumbled over this problem, because I use a datastructure with this reflexive property in my get_bonds method. I think there are good reasons for both cases, but will comply with any specification you do in your project. I made a pull request for stating this more clearly in the specification file.

mcocdawc commented 6 years ago

Thanks for accepting the PR ;)

cryos commented 6 years ago

Sounds good, I merged your change, I never really considered them to be directional things, and I think most code I work on implicitly models the fact that the bond is between two things, and has no directional nature.