#563 Patch for a isolated uncharged Se atom

Accepted
closed
nobody
None
cdk-1.4.x
1
2012-11-07
2012-10-30
No

Incorporating Gilleain's patch from bug #1201, rebasing on the latest cdk-1.4.x and with more stringent conditions on the number of implicit hydrogens.

1 Attachments

Discussion

  • John May

    John May - 2012-11-04

    I'm happy with 0001 and signed commit is on my accepted branch: https://github.com/johnmay/cdk/tree/cdk-1.4.x-accepted

    With 0002 just wanted to check a couple of things
    * the implicit Hydrogen count is null by default so perhaps this should have a null check before the test of 0. Other atom types seem to do atom.getImplicitHydrogenCount() == null || atom.getImplicitHydrogenCount() == 0.
    * if the atom typing depends on implicit hydrogens but the implicit hydrogens depends on atom typing - which comes first? I guess it's a similar problem to aromatic information in the atom typing so is this Se.2 atom type only if the implicit hydrogen count is set when it's read?

    ~~~~~:::Java
    if (atom.getImplicitHydrogenCount() == 0) {
    IAtomType type = getAtomType("Se.2");
    if (isAcceptable(atom, atomContainer, type)) return type;
    } else {
    IAtomType type = getAtomType("Se.3");
    if (isAcceptable(atom, atomContainer, type)) return type;
    }
    ~~~~~

    Cheers,
    J

     
  • John May

    John May - 2012-11-04
    • milestone: Needs_Review --> Needs_Revision
     
  • Egon Willighagen

    1. atom.getImplicitHydrogenCount() == null || atom.getImplicitHydrogenCount() == 0

    -> Ah, yes, that should have been the other way around, which does the same as the above but in one go, without the NPE chance:

    0 == atom.getImplicitHydrogenCount()

    1. The implicit hydrogen count can be part of the input, e.g. from MDL molfiles.

    So, I will apply 1.

    BTW you removed your cdk-1.4.x-accepted branch.

     
  • John May

    John May - 2012-11-06

    The update will still cause a NPE - using a non null one on the left only works if the you use 'Object.equals()'. I was pretty sure but tested just incase and it does throws a NPE.

    ~~~~~:::Java
    Integer implicitH = null;
    System.out.println(0 == implicitH); // NPE

    you can do this though:
    
    ~~~~~:::Java
        Integer.valueOf(0).equals(implicitH)
    

    and due to integer caching they'll be no object creation

    ~~~~~:::Java
    public static Integer valueOf(int i) {
    if(i >= -128 && i <= IntegerCache.high)
    return IntegerCache.cache[i + 128];
    else
    return new Integer(i);
    }
    ~~~~~

     
    Last edit: John May 2012-11-06
  • John May

    John May - 2012-11-06

    I have a feeling it works in groovy...

     
  • Egon Willighagen

    Both patches are now linked to the "Darn" comment... please review.

     
  • John May

    John May - 2012-11-06

    Okay thanks - will check them out first thing tomorrow.

     
  • John May

    John May - 2012-11-07

    All good - there on cdk-1.4.x-accepted. It was missing as there were no non-accepted commits on there so a deleted an merged for cdk-1.4.x again to keep the history clean - I forgot to repush when I signed off orginally.

    J

     
  • Egon Willighagen

    • status: open --> closed
    • milestone: Needs_Revision --> Accepted
     
  • Egon Willighagen

    John reviewed it, I tweaked, and it got approved, applied, and pushed.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks