Colorize dendrogram #48

stefanhahmann · 2023-11-02T13:59:49Z

Adds colorizing the dendrogram with the same colors that are used for the tag sets

Resolves #49

maarzt

Nice functionality. I tested the "cluster lineage tree" plugin and it works as expected.
The test coverage is impressive. Which gives me confidence that the code will run reliably.

The code quality is goof. But I see one mayor point for improvement. This PR adds new methods to the "Classification" class. The class became a bit unorganized. I think restructuring it would be valuable.

Regarding javadoc & comments: There are sometimes concepts or surprises in the code. That are nowhere described, for example:

The class ID in the Classification class, what does it refer to.
We no the the "WeightedLinkageStrategy" class is named wrongly. And therefor we decided extend the class. This should be described in the commet.
That a class is a copy of a class from another repository.
There are tests that only test a side effect "CustomizedClusterComponentTest"

Please add, one sentence of javadoc to each class in the repository. One sentence that describes the purpose of class.

There is a small problem the color of the 4th class is always black. This is due to the use of the glasby lookup table and the fact that the first 5 colors in there are "outliers colors". I would suggest to exclude the first five colors.

src/main/java/org/mastodon/mamut/clustering/ui/CustomizedClusterComponent.java

src/main/java/org/mastodon/mamut/clustering/ui/DendrogramPanel.java

src/test/java/org/mastodon/mamut/clustering/ui/CustomizedClusterComponentTest.java

src/test/java/org/mastodon/mamut/clustering/ui/DendrogramPanelTest.java

src/main/java/org/mastodon/mamut/clustering/util/Classification.java

src/test/java/org/mastodon/mamut/clustering/ui/DendrogramPanelDemo.java

src/test/java/org/mastodon/mamut/clustering/ClusterApporientedDemo.java

* Extends existing ClusterComponent with a Color specific to it * The color is set before the paint method of super class ClusterComponent is called

…r palette

…lasses * classified objects are stored in an (ordered) list instead of a set * a mapping between cluster objects and cluster ids is added

…Test

…from class id to colors and a separate mapping from Cluster to color

…air<Set, Cluster>

…n ordered list of classifications consisting of a set of classified objects and a Cluster object belonging to it

…Controller

…f, leafmapping * Use the new DendrogramView constructor in ClusterRootNodesController

…lebar * Painting the cutoff line and adapting the scalebar is now completely done in the DendrogramPanel

…the cluster object is not null

…jects cluster, mapping and colors

…classification object

…for it

…r testability

* The private variable children of the parent class ClusterComponent is initialized in CustomizedClusterComponent constructor by calling super.getChildren()

* Make field CLUSTER_LINE_COLOR package protected so that there is an assumption that can be tested

…l to CustomizedClusterComponent class

* This class names better fits it purpose

…rogramPanelTest class

…structor of CustomizedClusterComponent. This is sufficient, since the constructor is the only public method that uses T.

…needs a cluster object and a classification object as parameters

This includes: - omit the obscure "leaf name" variable -> the leave mapping is now performed already in the ClusterUtils methods - omit any cluster ids - renamed `algorithmResult` to `rootCluster`, since this is a more appropriate name

…ification

… for List to Set * List is not required, since ordering is not relevant

…rategy explaining why they override existing classes from the clustering library

…nce the 4th color is close to black and thus difficult to see

sonarcloud · 2023-11-13T11:08:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

91.9% Coverage
0.0% Duplication

stefanhahmann · 2023-11-13T11:10:04Z

Nice functionality. I tested the "cluster lineage tree" plugin and it works as expected. The test coverage is impressive. Which gives me confidence that the code will run reliably.

The code quality is goof. But I see one mayor point for improvement. This PR adds new methods to the "Classification" class. The class became a bit unorganized. I think restructuring it would be valuable.

Regarding javadoc & comments: There are sometimes concepts or surprises in the code. That are nowhere described, for example:
* The class ID in the Classification class, what does it refer to.

* We no the the "WeightedLinkageStrategy" class is named wrongly. And therefor we decided extend the class. This should be described in the commet.

* That a class is a copy of a class from another repository.

* There are  tests that only test a side effect "CustomizedClusterComponentTest"
Please add, one sentence of javadoc to each class in the repository. One sentence that describes the purpose of class.

There is a small problem the color of the 4th class is always black. This is due to the use of the glasby lookup table and the fact that the first 5 colors in there are "outliers colors". I would suggest to exclude the first five colors.

Thanks for the review.

I did the following steps:

restructured the Classification class so that the IDs are no longer needed
Added a comment explaining that WeightedLinkageStrategy was overridden, because it is wrong
Added comments to the copied (or rather re-implemented classes)
Explained that CustomizedClusterComponentTest has only limited testing value
Added a javadoc to each new class
Skipped the first five colors of the Glasbey color pallette.

stefanhahmann force-pushed the colorize_dendrogram branch from 8ef1576 to 7c38107 Compare November 2, 2023 14:05

stefanhahmann self-assigned this Nov 3, 2023

stefanhahmann force-pushed the colorize_dendrogram branch 2 times, most recently from 3e53573 to 3c92578 Compare November 7, 2023 13:06

maarzt reviewed Nov 9, 2023

View reviewed changes

stefanhahmann added 25 commits November 9, 2023 15:56

Add new class CustomizedClusterComponent

675dc35

* Extends existing ClusterComponent with a Color specific to it * The color is set before the paint method of super class ClusterComponent is called

Add methods to ClusterUtils to pick some Colors from the Glasbey colo…

f1d1d49

…r palette

Modify class Classification to enable the identification of cluster c…

b513c83

…lasses * classified objects are stored in an (ordered) list instead of a set * a mapping between cluster objects and cluster ids is added

Remove no longer required classes DendrogramUtils and DendrogramUtils…

0fbb7ee

…Test

Handle values <= zero for method getGlasbeyColors()

c3b9d52

Rename objectMapping to leafMapping in class Classification

c641d60

Change class Classification such that it is able to return a mapping …

36e7bd0

…from class id to colors and a separate mapping from Cluster to color

Change param classifiedObjects of class Classification to a List of P…

aa670da

…air<Set, Cluster>

Change method getGlasbeyColors() to return a List instead of a Map

febcceb

Remove unrequired import from ClusterUtilsTest

51b2a90

Rework ClusterUtils to create a Classification object that contains a…

271cb80

…n ordered list of classifications consisting of a set of classified objects and a Cluster object belonging to it

Change the way how colors are assigned to tagsets in ClusterRootNodes…

e39df91

…Controller

Use classification object in DendrogramView instead of cluster, cutof…

c4a2c0f

…f, leafmapping * Use the new DendrogramView constructor in ClusterRootNodesController

Remove DendrogramPanelWithCutoffLine from DendrogramView and adaptSca…

34524f7

…lebar * Painting the cutoff line and adapting the scalebar is now completely done in the DendrogramPanel

Ensure adapting the scale bar in the DendrogramPanel is only done if …

94e1291

…the cluster object is not null

Use Classification object in DendrogramPanel instead of individual ob…

cd21e0f

…jects cluster, mapping and colors

Add basic test to check if a DendrogramPanel can be generated with a …

4df9636

…classification object

Extract Scalebar of DendrogramPanel to sub class and add a unit test …

8b9d06a

…for it

Let createComponent return null immediately if cluster is null

e25b9f9

Use Graphics2D instead of Graphics where applicable

0c4c5fd

Use static CUT_OFF_LINE_STROKE

5b0fdd1

Extract no data string to static variable

2b235f7

Introduce new method getVerticalLine and unit test for it

d20291d

Change DisplayMetrics inner class of DendrogramPanel to achieve bette…

f65ec87

…r testability

Make paint method of Scalebar private

f25ba4f

stefanhahmann added 7 commits November 9, 2023 15:56

Add a basic test for the CustomizedClusterComponent

88c4a55

Extend CustomizedClusterComponentTest to test paint method

a41af38

* The private variable children of the parent class ClusterComponent is initialized in CustomizedClusterComponent constructor by calling super.getChildren()

Reuse classification in Test from class instead of creating a new one

1c2a2d2

Include a basic test of the paint method of the DendrogramPanel

64d8323

* Make field CLUSTER_LINE_COLOR package protected so that there is an assumption that can be tested

Move initialization of CustomizedClusterComponent from DendrogramPane…

b659fac

…l to CustomizedClusterComponent class

Add some javadoc

d4c186a

Add final modifiers to some method variables that were missing

3345cc6

stefanhahmann force-pushed the colorize_dendrogram branch from 3c92578 to 3345cc6 Compare November 9, 2023 14:56

stefanhahmann added 12 commits November 9, 2023 16:02

Change name of class ScaleBar and some related variables/methods to Axis

b8e2fdf

* This class names better fits it purpose

Add a description to class ClusterApporientedDemo

4ee01db

Add a description to class DendrogramPanelDemo

bc81b77

Add comment re the reason for mocking the graphics object in the Dend…

31d243b

…rogramPanelTest class

Add some explanation to CustomizedClusterComponentTest

b7e6203

Convert the class type parameter T to a Wildcard parameter in the con…

7119f70

…structor of CustomizedClusterComponent. This is sufficient, since the constructor is the only public method that uses T.

Add some javadoc to CustomizedClusterComponent class

27c9f84

Introduce a new constructor for CustomizedClusterComponent that only …

45a51ac

…needs a cluster object and a classification object as parameters

Restructure class classification:

3dead27

This includes: - omit the obscure "leaf name" variable -> the leave mapping is now performed already in the ClusterUtils methods - omit any cluster ids - renamed `algorithmResult` to `rootCluster`, since this is a more appropriate name

Make method getClassifiedObjects() package protected

7e136af

Rename subclass ColoredCluster to ObjectClassification in class Class…

53c7157

…ification

Change type of variable objectClassifications in class Classification…

de76fa3

… for List to Set * List is not required, since ordering is not relevant

stefanhahmann force-pushed the colorize_dendrogram branch from 95d2686 to de76fa3 Compare November 10, 2023 15:44

stefanhahmann added 5 commits November 13, 2023 11:02

Add more javadoc to DendrogramPanel class

ed1c892

Add comments to AverageLinkageUPGMAStrategy and AverageLinkageWPGMASt…

30b776c

…rategy explaining why they override existing classes from the clustering library

Add missing javadoc to DendrogramView and Classification classes

00efbb5

Rename test in CustomizedClusterComponent to testPaint

bb9013d

Skip the first 5 colors of the Glasbey palette for the dendrogram, si…

333b8dd

…nce the 4th color is close to black and thus difficult to see

stefanhahmann merged commit fc4fa9f into master Nov 13, 2023
4 checks passed

stefanhahmann deleted the colorize_dendrogram branch November 13, 2023 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorize dendrogram #48

Colorize dendrogram #48

stefanhahmann commented Nov 2, 2023 •

edited

Loading

maarzt left a comment

sonarcloud bot commented Nov 13, 2023

stefanhahmann commented Nov 13, 2023

Colorize dendrogram #48

Colorize dendrogram #48

Conversation

stefanhahmann commented Nov 2, 2023 • edited Loading

maarzt left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 13, 2023

stefanhahmann commented Nov 13, 2023

stefanhahmann commented Nov 2, 2023 •

edited

Loading