Repository navigation
More performant interface clustering - #1155
Conversation
aalhossary
left a comment
There was a problem hiding this comment.
LGTM
I just had 4 comments inline and one below:
getAverageScore() is now the average of representative merge scores, not "all pairs of members". StructureInterfaceCluster.java:37 and :80 still say the old thing
| List<StructureInterface> reps = new ArrayList<>(n); | ||
| List<Pair<T>> idPairs = new ArrayList<>(n); | ||
| for (StructureInterfaceCluster cluster : initialClusters) { | ||
| StructureInterface rep = cluster.getMembers().get(0); |
There was a problem hiding this comment.
members may be empty.
get(0) needs guarding or an explicit IllegalArgumentException
| * By default it is ignored. | ||
| * To execute use: | ||
| * <pre> | ||
| * mvn -Dtest=TestInterfaceClusteringPerformance test |
There was a problem hiding this comment.
will -Dtest=TestInterfaceClusteringPerformance on the class override the @ignore on testClusteringPerformance()? worth double checking.
There was a problem hiding this comment.
It looks like it wouldn't be possible to run with maven CLI as things are (without commenting out the @Ignored). I've fixed the javadoc to say it is easily doable from an IDE instead: 204dd7b
| * <p> | ||
| * Interfaces are compared by entity pairs (see {@link #ENTITY_ID_PAIR}) and grouped with the leader | ||
| * algorithm described in {@link #clusterInterfaces(List, Function, double)}, thus the result depends on | ||
| * the order of the list (by default descending by area, see {@link #sort()}). |
There was a problem hiding this comment.
Here is what Claude noted:
Result now depends on list order, but getClusters() doesn't sort and caches forever. TestInterfaceClustering.test1AUY:121 clusters before calcAsas() — all areas 0, leaders arbitrary, and that result is cached for lines 146/163. Suggest calling sort() at the top, or warning when all areas are 0.
There was a problem hiding this comment.
I've now forced a sort by number of contacts (areas are not available at that point) to make the clustering more deterministic: f2a9c1b
| cluster.getMembers().addAll(initialClusters.get(i).getMembers()); | ||
| double sumScores = 0; | ||
| int countMerges = 0; | ||
| // descending, so that merged members are appended in the same order as in previous implementations |
There was a problem hiding this comment.
According to Claude:
"previous implementations" appended members in ascending index order (
SingleLinkageClusterercollects into aTreeSet), so descending reverses it rather than preserving it — So [1,2,3] becomes [1,3,2].
Since direction can't change which clusters merge (same contents but different order),for (int j = i + 1; j < n; j++)gets the stated compatibility for free. (Nothing asserts member order, so the tests pass either way.)
aalhossary
left a comment
There was a problem hiding this comment.
LGTM.
I just had 4 comments online, in addition to the below one (can't be inline):
getAverageScore() is now the average of representative merge scores, not "all pairs of members". StructureInterfaceCluster.java:37 and :80 still say the old thing.
Also bugfix: reset clusters after trimming.
Also bugfix: reset clusters after trimming.
I've fixed the javadocs: 0dff0c6 |
Using a leader clustering algorithm. Works well in all cases I know for the task of interface clustering, be it for all interfaces of an assembly or all interfaces found in a crystal. The performance difference from the previous version is stark for cases with many interfaces, see TestInterfaceClusteringPerformance