search

open-reaction-database / ord-schema

Schema for the Open Reaction Database
https://open-reaction-database.org
Apache License 2.0
95 stars 27 forks source link

This would be good to add to e.g. `message_helpers.py` in another PR. #494

Closed skearnes closed 3 years ago

skearnes commented 4 years ago

This would be good to add to e.g. message_helpers.py in another PR.

_Originally posted by @skearnes in https://github.com/Open-Reaction-Database/ord-schema/pull/477#discussion_r508085524_

michaelmaser commented 3 years ago

Couple questions for thoughts on the dative bond message helpers:

  1. Looking to be consistent with naming, would something like get_dative_bonds be appropriate?

  2. Would it be preferable to have this operate directly on SMILES, where it could be used in preprocessing or on a Compound's SMILES Identifier, or should the input be a Compound message?

  3. Would it be preferable to have the check functions is/has_transition_metal embedded in get_dative_bonds, or have them be accessible as higher level functions?

@skearnes @connorcoley

skearnes commented 3 years ago

Couple questions for thoughts on the dative bond message helpers:

  1. Looking to be consistent with naming, would something like get_dative_bonds be appropriate?

I think set_dative_bonds is fine; this is actually changing the content so it's not really a "getter".

  1. Would it be preferable to have this operate directly on SMILES, where it could be used in preprocessing or on a Compound's SMILES Identifier, or should the input be a Compound message?

I think operating on an RDKit Mol like you have now is fine. Then it can come from various identifiers via mol_from_compound.

  1. Would it be preferable to have the check functions is/has_transition_metal embedded in get_dative_bonds, or have them be accessible as higher level functions?

Separate free functions, please. Makes it easier to test :)

michaelmaser commented 3 years ago

Sounds great, thanks!

michaelmaser commented 3 years ago

Closed by #501