Skip to content

Proposal for storing bonding information#23

Open
mkhorton wants to merge 9 commits intoMolSSI:masterfrom
mkhorton:topology-update
Open

Proposal for storing bonding information#23
mkhorton wants to merge 9 commits intoMolSSI:masterfrom
mkhorton:topology-update

Conversation

@mkhorton
Copy link
Copy Markdown

@mkhorton mkhorton commented Dec 1, 2017

This is following the 'big system' approach of large arrays containing information for all atoms.

Comment thread Topology/README.md Outdated

For metadata, there are a few pre-defined keys with strict meanings:

* `bond_type` can take values `single`, `double`, `triple` [to replace -- this is just an example], only suitable for connections involving pairs of atoms
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Angle and torsions would need to be handled in a similar way

bond_type defined as is may not be necessary. Single, double, triple ... bonds are defined through the set of parameters associated with each atom type

Comment thread Topology/README.md Outdated

For metadata, there are a few pre-defined keys with strict meanings:

* `bond_type` can take values `single`, `double`, `triple` [to replace -- this is just an example], only suitable for connections involving pairs of atoms
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Angle and torsions would need to be handled in a similar way

bond_type defined as is may not be necessary. Single, double, triple ... bonds are defined through the set of parameters associated with each atom type

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed -- I don't know if bond_type specifically is necessary, I included it as an example. I think some pre-defined keys might be useful for GUI/visualization though.

Comment thread Topology/README.md
Each connection is defined as:

* `indices`, an ordered list of length `N` of atomic indexes defining the connection
* `images`, (optional, only relevant if a unit cell lattice is defined) an ordered list of length `3N` describing the periodic image of the connected atom -- by default, this is assumed to be (0,0,0), meaning that the connected atom is in the origin image, and first index is set to (0,0,0) by convention
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not a list of lists like for gemoetry and unit_cell?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'd like that -- it'd make it a lot more human readable. I'm not sure if it'd have consequences for file size/loading time for large systems though.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right, but geometry is currently specified as a list of lists, and the same issue applies there...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree it should be consistent with geometry, I didn't realize this was list of lists.

@cryos
Copy link
Copy Markdown
Collaborator

cryos commented Dec 1, 2017

Looks good

Comment thread Topology/README.md

For metadata, there are a few pre-defined keys with strict meanings:

* `bond_type` can take values `single`, `double`, `triple` [to replace -- this is just an example], only suitable for connections involving pairs of atoms, this is useful for GUI/visualization tools
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What about order, and use an integer rather than a string, or are there other types being envisaged?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Order sounds sensible! bond_type was just an example to get the conversation started: some pre-defined keys would be good, I'm (personally) agnostic as to what these keys should be.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

But order and an integer value sounds good.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sounds like geometry should be changed following discussions from yesterday. I can make a pull request.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Any real objection to adding bond_types as floats to allow slightly more flexibility for say aromatic compounds.

@cryos
Copy link
Copy Markdown
Collaborator

cryos commented Dec 1, 2017

Added #28 to make these more uniform

kimt33 and others added 4 commits December 1, 2017 14:40
1. Change field `masses` to mass of molecule to mass of atoms. I think
this was a typo.
2. Add fields `num_protons`, `num_electrons`, and `basis`. The ghost and
dummy atoms can be constructed with the appropriate selection of these
fields (plus `masses`)
1. Add examples for symbols, geometry, name, multiplicity, masses,
num_protons, num_electrons, basis, comment, fragments, fragment_charges,
and fragment_multiplicities,
Add more specifications for each atom
Copy link
Copy Markdown
Collaborator

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

Looks good, @cryos can you make the mentioned PR?

@dgasmith
Copy link
Copy Markdown
Collaborator

@mkhorton The schema layout has changed quite a bit since this PR, apologies! If you can supply 1-2 JSON examples I would be more than happy to work on incorporating this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants