#544 CMLReader Patches

Needs_Review
closed
nobody
cdk-1.4.x (181)
8
2014-08-11
2012-09-24
John May
No

Patch fixes two bugs on the CMLReader (actually it's the CMLCoreModule convention). I have implement two tests to demonstrate the bugs and then provided their respective fixes. I'd consider both bugs pretty major (hence priority 8 :D) as you can't really use CML is the given 'flavour' unless you manually modify it.

Atomic number defaulting to 1: https://sourceforge.net/tracker/?func=detail&aid=3553328&group_id=20024&atid=120024
Content of bondStereo ignored: https://sourceforge.net/tracker/?func=detail&aid=3557907&group_id=20024&atid=120024

Discussion

  • Ralf Stephan

    Ralf Stephan - 2012-09-25

    Looks good. Maybe it even resolves some known JCP issue. Please go ahead.

     
  • John May

    John May - 2012-09-25

    Could you sign off the commits please,

    Thanks

     
  • Ralf Stephan

    Ralf Stephan - 2012-09-25

    You mean hit the Accepted button of this issue? I haven't the rights!

     
  • John May

    John May - 2012-09-25

    Not sure if there is a page on the wiki for this - should probably do a tutorial on it... You need to sign it with git. Then when you apply the patches to check the tests you can sign it off as follows.

    git am patch-file.patch

    if the patch doesn't apply (normally whitespace) you need to abort and try again ignore white space

    git am --abort
    git am --ignore-whitespace patch-file.patch

    then once everything is checked you can sign off the previous commit using

    git commit --amend --signoff

    Egon can probably explain it better but heres the way I understand it:

    The signoff shows you've applied and checked the patches don't break any tests and perform as expected. If you have a look at the egonw/cdk/master git branch you'll see each commit is signed off by someone else (current master was a commit by me signed by Egon). This serves as a peer review on code - it's seems a bit of pain at first but actually once you do it once it's very quick to do the merging and the time is dependant on the patch size. When a reviewer catches something you missed it pays off and actually the point you see it's benefit.

    I believe the signing off model comes from the Linux Kernal (Linus doesn't trust many) but the theory is that if you really want to change something in the main branch it needs to be confirmed it doesn't break. If you have a lot of changes to make quickly and can't wait for them to be reviewed (i.e. JChemPaint fork) you can branch off and carry on working (making sure you keep in sync with the upstream repo) and merge back later.

     
  • Egon Willighagen

    [wiki tutorial] yes, would be great.

    I can also recommend the "git am -3" option, which is for three-way merging... that tends to help me too.

     
  • Ralf Stephan

    Ralf Stephan - 2012-09-26

    Well, it's all fine locally on my machine now, thanks. It's not clear however how I push this to cdk/cdk without making a pull request via rwst/cdk, as Egon requested to me.

     
  • Egon Willighagen

    Ralf,

    pull requests are fine. We need to consolidate in a single place where we let each other know what there is to pull in, though.... I now seem to have at least four places where patches can occur... that is too much, but if you link a pull request from here, that is perfect... I like this tracker to be the consolidated tracker for seeing what is there to review/update/apply... then it does not matter where the patches physically are, as long as they are linked from here.

    And thanx for the reviewing!

     
  • Egon Willighagen

    John, didn't you make a unit test too?

    Ralf, when I look at pull29, I only see the fixes, but no unit tests...

     
  • Ralf Stephan

    Ralf Stephan - 2012-09-26

    argl, appended. That's an advantage of pull requests...

     
  • Egon Willighagen

    OK, cool thanx!

     
  • Egon Willighagen

    Mmm... OK, one unit test keeps failing, and when I apply the patch, there is a missing import :(

    Sorry, I cannot look further into it just now... but applying it to cdk-1.4.x does currently not work... not sure what went wrong...

     
  • Ralf Stephan

    Ralf Stephan - 2012-09-26

    I can tell what went wrong on my side: I never ran the test. I'm not used to CDK development and CDK testing in particular. I know, John explained to me how to test, but I didn't. That's my half of the issue.

     
  • John May

    John May - 2012-09-26

    Hmm very strange - I def tested and all was working but tested again now and it's still broken.

    I can also explain what happened... I had a previous patch that I rebased and missed the import off. As the test fail I realise my fix didn't actually work and the problems go deeper then I thought. It is possible to fix but not well.

    My fix was that when there is a symbol and no atomic numbers the atomic numbers is set from the symbol. However looking now I see the atomic numbers are always set.....

    So when the "atom" element has finished reading it does the following (to all internal type arrays).
    ...
    if (atomCounter > atomicNumbers.size()) {
    atomicNumbers.add(null);
    }
    ...

    then when the data is stored/converted the heuristic to check is:

    if (atomicNumbers.size() == atomCounter) {
    hasAtomicNumbers = true;
    }

    so for "most" data types the size will always equal the atom count as it the number of elements was increased to meet the atom count...

    which is probably why there is null checks when the array is actually read

    if (hasAtomicNumbers) {
              if (atomicNumbers.get(i) != null)
                currentAtom.setAtomicNumber(Integer.parseInt(atomicNumbers.get(i)));
            }
    

    anyways i've already fixed it so will re-upload. sorry for the issues

     
  • John May

    John May - 2012-09-26

    Okay have fixed the implementation with the import and the atomic numbers are now correctly read.

    File has been reloaded to tracker

     
  • Ralf Stephan

    Ralf Stephan - 2012-09-27

    OK. John, I get here:

    junit-test:
    [echo] JUnit tests class: testclass=io.CMLReaderTest [JVM Arguments: cdk.debugging=false, true]
    [junit] Running org.openscience.cdk.io.CMLReaderTest
    [junit] Tests run: 16, Failures: 1, Errors: 0, Time elapsed: 0.472 sec

    report/...etc:
    Testcase: testAcceptsAtLeastOneNonotifyObject(org.openscience.cdk.io.CMLReaderTest): FAILED
    junit.framework.AssertionFailedError: At least one of the following IChemObect's should be accepted: IChemFile, IChemModel, IMolecule, IReaction
    at org.openscience.cdk.io.ChemObjectIOTest.testAcceptsAtLeastOneNonotifyObject(ChemObjectIOTest.java:91)

     
  • Egon Willighagen

    Thanx for resolving the issue!

     

Log in to post a comment.

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

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks