-
Notifications
You must be signed in to change notification settings - Fork 5
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
Random coloring of lineage trees only for vizu enhancement #29
base: master
Are you sure you want to change the base?
Random coloring of lineage trees only for vizu enhancement #29
Conversation
Hi Vlado, This is a nice feature! Normally I would make a color scheme based on the track index to create vizu that depends on the track of spots. But right now the color scheme we have interpolate between scarce color. Another approach that would circumvent the need to create tags would be to see if we can retrofit a Glasbey LUT on the Colormap class: If the Do you think it could be extended to do so? |
I didn't know we can draw spots with different colors, I always thought the coloring is either following chosen TagSet or uses some one default color (albeit user-changeable via the preferences dialog) I have to investigate, ATM I'm not 100% sure I understand all you're saying.. conclusion: I'll try the thanks for the pointers! |
I have the feeling @tinevez had in mind a similar in looking, yet different coloring principle. While I was proposing a persistent, robust to changes (at least to track deletion) solution based on tagset, Jean-Yves probably meant an (optional feature of) online/on-the-fly coloring of spots and edges (when no tagset is selected/activated) during their drawing in the TrackScheme canvas and elsewhere (but especially not outside Mastodon itself). If that is the case, I would then merge this PR and create an issue in the Mastodon main repo to decide how the on-the-fly solution could look like. What do you think @tinevez ? |
If the on-the-fly coloring would be based on some track index, which tells the order of the track to be currently displayed, changes in the lineage could often lead to complete "recoloring" of even not-changed parts. I would prefer a variant where the color is a function of the track root spot label/ID... facilitates thinking like "the blue track I reviewed, let's now work on the red one" (and no matter what one does to the "red one", the "blue one" stays blue) |
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.
This PR adds a nice functionality to assign some randomly selected color from a predefined palette to colorize lineage trees randomly. This may be useful for some use cases.
I have tested the functionality and it works as expected.
However, it is unclear to me, why in this dialog, the existing tag sets are shown:
I understand the 2 options on the bottom (creating tagsets with random colors), but the other options in this select box I do not understand how they would work. When testing, I could not see the effect of choosing existing tagsets, but maybe I was doing something wrong.
/** | ||
* Help creating a visible color value by adding | ||
* a (full opacity) alpha channel. | ||
* @param rgbAsBottom24bits The encoded RGB triplet. | ||
* @return The int value of the RGB color, ready to use | ||
* with {@link org.mastodon.model.tag.TagSetStructure.Tag}. | ||
*/ | ||
static public int rgbToValidColor(final int rgbAsBottom24bits) { | ||
return 0xFF000000 | rgbAsBottom24bits; | ||
} | ||
|
||
/** | ||
* Help creating a visible color value by adding | ||
* a (full opacity) alpha channel. | ||
* @param r 0-255 valued red channel. | ||
* @param g 0-255 valued green channel. | ||
* @param b 0-255 valued blue channel. | ||
* @return The int value of the RGB color, ready to use | ||
* with {@link org.mastodon.model.tag.TagSetStructure.Tag}. | ||
*/ | ||
static public int rgbToValidColor(final int r, final int g, final int b) { | ||
return 0xFF000000 | ((r & 0xFF) << 16) | ((g & 0xFF) << 8) | (b & 0xFF); | ||
} | ||
|
||
/** | ||
* Help creating a fully-described color value. | ||
* @param r 0-255 valued red channel. | ||
* @param g 0-255 valued green channel. | ||
* @param b 0-255 valued blue channel. | ||
* @param alpha 0-255 valued opacity (alpha) channel, | ||
* 0 - fully transparent, 255 - fully opaque. | ||
* @return The int value of the RGB color, ready to use | ||
* with {@link org.mastodon.model.tag.TagSetStructure.Tag}. | ||
*/ | ||
static public int rgbaToValidColor(final int r, final int g, final int b, final int alpha) { | ||
return ((alpha & 0xFF) << 24) | ((r & 0xFF) << 16) | ((g & 0xFF) << 8) | (b & 0xFF); | ||
} | ||
|
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.
-
The class TagSetUtils had been moved to: https://github.com/mastodon-sc/mastodon/blob/dev/src/main/java/org/mastodon/util/TagSetUtils.java
-
It would be good to make a Pull Request to change the class in that location.
-
Please change the order of the method modifiers.
The Java Language Specification recommends listing modifiers in the following order:
Annotations
public
protected
private
abstract
static
final
transient
volatile
synchronized
native
default
Not following this convention has no technical impact, but will reduce the code’s readability because most developers are used to the standard order. -
There is also a class called ColorUtils https://github.com/mastodon-sc/mastodon/blob/dev/src/main/java/org/mastodon/util/ColorUtils.java You could consider moving these methods there, since they are more about colors than about TagSets.
-
You could consider providing unit tests for each of these methods.
private LogService logService; | ||
|
||
@Parameter(label = "Choose color scheme:", initializer = "readAvailColors", choices = {}) | ||
private String colorScheme = "Create new tagset"; |
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.
Field may be final
@Parameter(label = "Choose color scheme:", initializer = "readAvailColors", choices = {}) | ||
private String colorScheme = "Create new tagset"; | ||
|
||
final static private String COLORS_16 = "Create and use a new tagset (16 colors)"; |
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.
Reorder the modifiers to comply with the Java Language Specification
final static private String COLORS_16 = "Create and use a new tagset (16 colors)"; | |
private static final String COLORS_16 = "Create and use a new tagset (16 colors)"; |
|
||
// sets up the drop-down box in the dialog with two create-tagset-items | ||
// followed by the list of the currently available tagsets | ||
private void readAvailColors() { |
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.
private void readAvailColors() { | |
@SuppressWarnings( "unused" ) | |
private void readAvailColors() { |
return chosenTagSet; | ||
} | ||
|
||
|
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.
@Override |
this.getInfo().getMutableInput( "colorScheme", String.class ).setChoices( choices ); | ||
} | ||
|
||
static final Map<String, Integer> mokolePaletteOf16Colors = new HashMap<>(16); |
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.
Consider adding this Palette to this class: https://github.com/mastodon-sc/mastodon/blob/dev/src/main/java/org/mastodon/util/ColorUtils.java
ColorUtils contains the Glasbey Palette already. I think it would be useful, if different color palettes would be in the same class or at least near to each other.
} | ||
|
||
static final Map<String, Integer> mokolePaletteOf16Colors = new HashMap<>(16); | ||
{ |
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.
{ | |
static { |
.getTagSetStructure() | ||
.getTagSets() | ||
.stream() | ||
.filter(_ts -> _ts.getName().equals(colorScheme)) |
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.
Could you use a different name here?
Variable names starting with "_" do not match the naming conventions of Java.
|
||
|
||
private TagSetStructure.TagSet getChosenTagSet() { | ||
TagSetStructure.TagSet chosenTagSet = null; |
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.
TagSetStructure.TagSet chosenTagSet = null; | |
TagSetStructure.TagSet chosenTagSet; |
for (int rColor = 0; rColor < rColors; ++rColor) | ||
for (int gColor = 0; gColor < gColors; ++gColor) | ||
for (int bColor = 0; bColor < bColors; ++bColor) { | ||
int color = ((rColor*rStep) << 16) + ((gColor*gStep) << 8) + bColor*bStep; |
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.
This line looks a bit as it could be rewritten using one of the existing rgbToValidColor() methods. Could you check this?
(besides I could really use to have this ability to colorize lineage in the Mastodon-sciview Bridge project) |
@stefanhahmann Thank you sincerely for your kind review of the code. I'll now react to the first question, the code-related suggestions I'll implement unf. later |
My intention was really only to visually set apart the individual trees (from the full lineage) and avoid tagging them manually. There's nothing more behind it. And I wanted a permanent assignment -- when testing some analysis code, the coloring can help one to get back to what she was inspecting before (the data reload).
The plugin essentially combines two things. In order to color individual trees, one needs a palette (tagset in this case) first, and then processes the lineage. For the former, either one can use an existing tagset or create a new one -- and that's the purpose of the dialog, choose from an existing or create a new one. And yes, the dialog is stupid not realizing that e.g. 16color palette has been already created and keeps offering to create it again and again. So, unless there is a mistake in the code (can always happen), choosing an existing tagset should be assigning colors from it (in a round-robin fashion). |
I am still not sure, if I understand the intended behavior correctly. Cf. e.g. the following example. The trees are colored with using a tagset. Then I apply the function from the plugin. After that the trees are colored exactly the same. Is this the intended behavior in this case or am I using the plugin in an unintended way? |
That was actually unintended behavior, but it's correct behavior given the state of the code :( The code is discovering the individual lineages (individual roots) in fact in deterministic order and it iteratively assigns colors to the lineages, so executing the code on the same lineage with the same tagset leads to exactly the same re-assignment, thus no visible change. But you are right the plugin name misleads the user to expect something different -- maybe I could, besides the requested code changes, insert in deed a bit of randomness into the process -- e.g. start with random index in the tagset (not always from 0 like it is now). So that, for example, calling the plugin on the same tagset again and again would yield different assignments (from still the same pool of colors). Maybe this should be an option of the plugin:
(I'm afraid I'll fix/finish this thing only during the hackathon in Brno... hope it's okay) |
Thanks for the clarification of the functionality of the plugin. The 2 suggested options sound useful to me. Regarding time schedule: I think there is no pressure. |
Hello. |
I'm aware of mastodon-sc/mastodon#254 , yet I think these two could live side by side the PR254 is on-the-fly coloring, while this one here is persistent assignment of tags. Visually, for the moment in time, they achieve the same thing but in the but this PR should use the code-base from the PR254, there seems to be a good stuff to reuse |
Hi,
this simple PR offers a menu command that allows a user to simply (and quickly) color every spot in the tracking data.
The user first chooses a TagSet to be used, or have a new TagSet created and then used. The plugin then takes Tags (colors) one by one, each color assigning to all spots within the same lineage tree, but switching colors between the trees.
The individual lineage trees are then visually enhanced, and that is the only purpose of this plugin.
As a side effect, this PR proposes to extend the
TagSetUtils
with three variants ofrgbToValidColor()
method. (I have originally forgotten about the alpha value and took me a little while to realize it... hope this helps for some next time.)Questions (in case this PR should make it to this repo):
Glasbey
class... new package with palettes? Some interface around them (consider on-the-fly generated palettes)?(FYI: @tinevez)