Skip to content
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

Merged
merged 8 commits into from May 1, 2017
Merged

Group refactor #308

merged 8 commits into from May 1, 2017

Conversation

gilleain
Copy link
Contributor

Refactor of the group (partition refinement) machinery. Does:

  • Move the logic to get connectivity from a molecule behind a Refinable interface
  • This allows removal of the different EquitableRefiner classes and abstract base class
  • This in turn simplifies the DiscreteRefiner hierarchy
  • Finally, the main functional point is to fix the equitable refinement for multigraphs (like molecules)
  • An Invariant interface provides two implementations, one for simple and one for multigraphs

Copy link
Member

@egonw egonw left a 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;
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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...)

Copy link
Contributor Author

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>
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add missing header.

@johnmay
Copy link
Member

johnmay commented Apr 28, 2017

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.

@johnmay
Copy link
Member

johnmay commented Apr 28, 2017

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.

@gilleain
Copy link
Contributor Author

Ok, given it a go. Now split into public interfaces, and a factory for creating refiners:

AtomContainerDiscretePartitionRefiner refiner = PartitionRefinement.forAtoms().create();

which is not exactly wonderful, but I couldn't think of a better mechanism.

@johnmay
Copy link
Member

johnmay commented May 1, 2017

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.

@johnmay johnmay merged commit 603e7f8 into cdk:master May 1, 2017
@gilleain gilleain deleted the groupRefactor branch May 1, 2017 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants