search

ideaconsult / cdk

The Chemistry Development Kit
https://cdk.github.io/
GNU Lesser General Public License v2.1
0 stars 0 forks source link

open TODO in RinchiGenerator #6

Closed uli-f closed 1 year ago

uli-f commented 1 year ago

There is a TODO 'handle allene atoms and double bonds' in RinchiGenerator:: cdkStereoElementToInchiStereo that was put there by you @ntk73.

ntk73 commented 1 year ago

@uli-f , This TODO was put in the code initially (the stereo specific issues around RInChI native library were not so clear at this moment). I see that is is a bit misleading. Generally the conversion of double bond and allene atom stereo elements is not needed for the proper work of RInChI generation (as it was noticed elsewhere, the stereo is handled via 2D/3D coordinates). I think, the only meaningful reason for having such a feature in the code is for code completeness of the conversion between CDK molecule data model and jna-rinchi molecule data model, but this feature would not be used in the RInChI handling process because such info is not put within MDL RXN/RDFile content by the native code.

I see following possible options to proceed:

  1. Remove TODO and leave this as a comment/explanation.
  2. (a). Implement the conversion code anyway (although most probably it will be never used). Here, the question arises: whether there will be a performance overload due to this addition. (b). Implement the conversion code with a configuration flag whether to apply this conversion or not (may be internally by default this check would be switched off and preserved for future uses while configuration flags would not be visible outside the class)

Do you have any preference how to proceed?, or you have another suggestion?

Similar is the approach for the other open TODO in class RInChIToReaction (issue #9)

uli-f commented 1 year ago

Thank you for the clarification!

I am happy to go with option 1, that is, removing the TODO and putting the explanation you gave in the github comment above somewhere into the code.

I'd probably prefer the explanation as a javadoc at the class level of RinchiGenerator and RinchiToReaction instead of a javadoc at the method level for the private methods cdkStereoElementToInchiStereo and inchiStereoToCDKStereoElement so that the explanation is easily accessible in the javadoc output.

I am a big fan of clearly pointing out any defaults, limitations etc in the documentation so that a user of the library knows about this.

ntk73 commented 1 year ago

I am a big fan of clearly pointing out any defaults, limitations etc in the documentation so that a user of the library knows about this.

I agree completely, I will do it. (this is a kind of programming code analogy of documenting a scientific methods with a proper meta data, protocols, conditions etc :-) ).

ntk73 commented 1 year ago

@uli-f , I have some hesitation about documenting this on a javadoc level because from the CDK user point of view. the conversion from jna-rinchi to CDK data model is a kind of transparent and such an "explanation" could be a little misleading.

uli-f commented 1 year ago

Okay, I see your point.

I am happy with putting this at the private method level so that it is more tailored towards developers than ordinary library users if that is what you prefer.

ntk73 commented 1 year ago

I have replaced the TODOs with extended comments (see 6f2d976df8c2b48b44c06fd9afd490e864f153bf). The same is done for issue #9

uli-f commented 1 year ago

Perfect, the inline comments make it very clear 👍🏼