#584 Second half of the group module/package

Accepted
closed
nobody
None
master
5
2012-12-02
2012-11-20
No

There are 4 patch files in the zip, but the first three are minor additions and a bugfix to the existing partition class.

The large patch contains all the new classes, test cases, and additions to depends files. Test-module runs without errors or fails. These patches are also available from this branch https://github.com/gilleain/cdk/tree/group

Also, there is a post explaining a bit about the module on my blog, although I won't spam the link here.

1 Attachments

Discussion

  • John May

    John May - 2012-11-22

    Nice one, Luis will be happy with the aromatic support. Going to try and get this reviewed tomorrow.

     
  • John May

    John May - 2012-11-26
    • milestone: Needs_Review --> Accepted
     
  • John May

    John May - 2012-11-26

    Looks great, signed commits are here: https://github.com/johnmay/cdk/tree/master-accepted.

    You were missing cdk-extra dep on the 0001 aromatic patch but I included that with a fixup as it wasn't worth another commit. There was a couple of missing '#' on the @link taglets which means the javadoc won't link correctly. I'll add comments to your branch where they are missing.

    Other then that some minor comments,

    • Permutation being a 'value' class can be an immutable final class. I tried it out and there would be no other changes. Also the JVM can optimise if it knows something is immutable.
    • DisjointSetForest was nice implementation, might be worth tagging it with a union-find keyword as it could be useful elsewhere. I was actually thinking the other day (with the disjoint SMILES) whether the connectivity checker could be better implemented using UF.
    • With HashSet/Map you should always try to give a guess at the size. The initial size is 16, when you hit 12 (75%) the entire map is rehashed. This happens every time you do a resize. You can easily provide the expected size using this pattern. Map<?> map = new HashMap<?>(n > 3 ? 1 + n + n/3 : n) - got this from guava. Obviously Trees don't suffer from this but you lose the near constant time access.
    • Is the initial partition defined once? - e.g. getElementPartition. If not you could gain some speed there by avoid strings and using atomic number with some bit shifting to get the invariance.
    • I saw you were using List<Map<Int,Int>> - we could definitely add some abstract Graph implementations for org.openscience.cdk.Graph. What do you think?
    • Some @TestMethod annotations were missing but they're redundant and the coverage check still identified the methods were tested.

    Egon, please push to master. Also please say if you think all the @TestMethod annotations need to be defined.

     
  • gilleain maclean torrance

    Excellent. I will add the missing #links and other changes as a patch to my branch, if that's ok.

    As for the other comments:
    1) Immutable permutation - good idea, I don't use 'final' nearly enough.
    2) Tag the DSF - yup.
    3) HashMap size guess - I saw you use that, and wondered about it. Seems reasonable.
    4) Atomic numbers in element partition - Probably a good idea anyway, since that should partition isotopes correctly. Maybe I'll make that a separate patch in the near future, with proper tests (ie : with isotopically labelled molecules).
    5) Graph implementations - Possibly. Depends what you mean by this.
    6) Missing TestMethod - Yeah, I noticed this yesterday, was wondering why they passed the coverage check. Guess that subclasses override them.

    Also, I'm reading through the line-notes, and will add those changes. Damn the word "partition" - I kept mistyping that!

     
  • John May

    John May - 2012-11-26

    3) Yeah - it's a shame the HashMap(int) constructor isn't more useful. The normal thing to do would be to say 'I have this many elements, give me a map that will hold them' instead of 'Give me a map of this size which will hold 75% of what I actually have'. I used to do something silly and pass the weight in as '100%' so it wouldn't do the resize but that is actually worse - the 75% minimises the bucket collisions and empty values so putting the weight to 100% means you're pretty much guaranteed collisions.
    5) I'll try and draft something up next week to show what I mean
    6) I believe you can omit them if you don't overload them - if you do have. getValue(Param1) and getValue(Param1, Param2) the checker finds two method names 'getValue' so it needs the explicit annotation.

    Oh and thanks for the great new module!

     
  • gilleain maclean torrance

    Right, the post-review patch is on the gilleain/cdk/group branch, along with a final commit that alters the data table in both refiners.

    Of course this was unwise, as it broke many unit tests, but they are now fixed. I might do some more work on the BondRefiner in future, as it is now a bit of a mess. But that will be in a while.

     
  • John May

    John May - 2012-11-26

    Just had a quick look and all looks okay. Will double check test/compilation in the morning but I'm sure it will be fine.

     
  • John May

    John May - 2012-11-29

    Okay all good. One think IntelliJ spotted was there is no '{@link #getGroup}' on the 'refine()' javadoc. Is this is a different class?

    Either way, Signed and pushed to 'master-accepted'

     
  • gilleain maclean torrance

    Argh. Dammit, yes : the whole point of the javadoc comment is that you can either call "class.getX(Y)" or "class.refine(Y); super.getX()" so that #getGroup should be a link to the superclass method.

    Anyway, thanks for the reviews!

     
  • gilleain maclean torrance

    Sorry, re-reading my last comment it may not have been clear. You are correct; it's my mistake. Could make a tiny patch for it.

     
  • John May

    John May - 2012-11-29

    Okay done

     
  • Egon Willighagen

    Applied and pushed.

    Added a missing dep in test-group.libdepends.

     
  • Egon Willighagen

    • status: open --> closed
     

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