#719 Fixing Add/Remove hydrogens

Needs_Review
closed
nobody
None
master
9
2014-01-19
2014-01-05
John May
No

Been meaning to get round to this for a while. This patch updates hydrogen adding and removal in the AtomContainerManipulator. The removeHydrogens now considers stereo-elements and so '[C@]([H])(C)(CC)O' can be correctly converted to '[C@H](C)(CC)O'. Also the method now checks whether a hydrogen can be represented as implicit. This changes the behaviour of molecular hydrogen which now does not return an empty container.

May also want to never remove bridging hydrogens but there is already a separate method for that.

https://github.com/johnmay/cdk/compare/cdk:master...patch%2Fmanipulator-hydrogen-handling

Discussion

  • John May
    John May
    2014-01-05

    • Description has changed:

    Diff:

    --- old
    +++ new
    @@ -1,4 +1,4 @@
    -Been meaning to get round to this for a while. This patch updates hydrogen adding and removal in the AtomContainerManipulator. The removeHydrogens not considers stereo-elements and so '[C@]([H])(C)(CC)O' can be correctly converted to '[C@H](C)(CC)O'. Also the method now checks whether a hydrogen can be represented as implicit. This changes the behaviour of molecular hydrogen which now does not return an empty container.
    +Been meaning to get round to this for a while. This patch updates hydrogen adding and removal in the AtomContainerManipulator. The removeHydrogens not considers stereo-elements and so '\[C@\](\[H\])(C)(CC)O' can be correctly converted to '\[C@H\](C)(CC)O'. Also the method now checks whether a hydrogen can be represented as implicit. This changes the behaviour of molecular hydrogen which now does not return an empty container.
    
     May also want to never remove bridging hydrogens but there is already a separate method for that.
    
     
  • John May
    John May
    2014-01-05

    Oh and adding hydrogens also updates the stereo elements.

     
  • John May
    John May
    2014-01-05

    • Description has changed:

    Diff:

    --- old
    +++ new
    @@ -1,4 +1,4 @@
    -Been meaning to get round to this for a while. This patch updates hydrogen adding and removal in the AtomContainerManipulator. The removeHydrogens not considers stereo-elements and so '\[C@\](\[H\])(C)(CC)O' can be correctly converted to '\[C@H\](C)(CC)O'. Also the method now checks whether a hydrogen can be represented as implicit. This changes the behaviour of molecular hydrogen which now does not return an empty container.
    +Been meaning to get round to this for a while. This patch updates hydrogen adding and removal in the AtomContainerManipulator. The removeHydrogens now considers stereo-elements and so '\[C@\](\[H\])(C)(CC)O' can be correctly converted to '\[C@H\](C)(CC)O'. Also the method now checks whether a hydrogen can be represented as implicit. This changes the behaviour of molecular hydrogen which now does not return an empty container.
    
     May also want to never remove bridging hydrogens but there is already a separate method for that.
    
     
  • John May
    John May
    2014-01-06

    • stereo elements are added with IAtomContainer.add(IAtomContainer) - the elements are now also stored as a Set. I couldn't bare to do 'contains()' on the list.
    • friendly clones of IAtom atom = org.clone(); and IBond bond = org.clone();
     
  • John May
    John May
    2014-01-08

    • Priority: 1 --> 9
     
  • John May
    John May
    2014-01-12

    One this has been patch I need to rewrite the hydrogen removal completely. Testing on some data and it's terribly slow thanks to 'getConnectedBondsMap'. I'll wait for this to be patched first though.

     
  • OK, looks good. But I do state that I personally find "original" better than "org", and "copy" better a variable name than "cpy"...

    Applied and pushed.

     
  • John May
    John May
    2014-01-19

    A subsequent patch has replaced the method meaning the 'cpy' is no longer used.

     
  • John May
    John May
    2014-01-19

    • status: open --> closed