New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Group refactor #308
Group refactor #308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see you continue on this work. Minor changes requested (administrative), and hope John can look at the algorithm a bit more?
@@ -0,0 +1,84 @@ | |||
package org.openscience.cdk.group; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the missing header.
} | ||
|
||
// TODO - neighbours in block test | ||
// @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use @ignore("TODO - neighbours in block test") ?
|
||
/** | ||
* @author maclean | ||
* @cdk.module group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double check with @johnmay, but I don't think we're using these anymore... (no action needed, but removing them is an option...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had forgotten about this - happy to remove them all, but haven't yet. Other changes made.
@@ -1,4 +1,4 @@ | |||
/* Copyright (C) 2012 Gilleain Torrance <gilleain.torrance@gmail.com> | |||
/* Copyright (C) 2017 Gilleain Torrance <gilleain.torrance@gmail.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or 2012-2017 or 2012,2017 ...
@@ -0,0 +1,187 @@ | |||
package org.openscience.cdk.group; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add missing header.
Yep will look into this at the weekend. Reminds of the n things I need to do also, I did external (in beam) work on canonical labelling myself. It would be good to integrate this all under one umbrella at some point.
|
Would it be possible to make the public classes package-private and encapsulate as much as possible behind a stable public API? Code looks fine but I don't like the API breakages, we can minimise this in future if the classes are package-private. |
Ok, given it a go. Now split into public interfaces, and a factory for creating refiners:
which is not exactly wonderful, but I couldn't think of a better mechanism. |
All good thanks for patches - please try to keep public method signatures intact in future but otherwise okay. I'll remove the print to standard out (AtomContainerPrinter) from the Azulene test. |
Refactor of the group (partition refinement) machinery. Does:
Refinable
interfaceInvariant
interface provides two implementations, one for simple and one for multigraphs