Gradient Coloring Option for Feature Metadata

[kwcantrell]

Address #405 This PR adds a gradient color option for feature metadata (similar to gradient option for barplots). There is one small issue I wasn't sure how to resolve though which you can see in the image below (look at the legend).

Basically, the gradient color bar extends beyond the initial size of the legend which triggers the scroll bar. I would like to have both the legend title and gradient color bar visible without triggering the scroll bar but there is some odd css that I can't figure out. @fedarko would you have any idea?

I will address the above issues in another PR. I want to get the basics of this PR merged in with master so that it would be possible to recreate the figure for the stacked faiths pd manuscript. This PR in its current form allows users to use a gradient color map on feature metadata. It will also display an error message to the user if they try to use gradient coloring on non-numeric data/only one unique number (similar to how barplots handle it). There is still an issue where gradient color triggers a scroll bar in the legend. But I will address that in a follow up PR (unless @fedarko or @ElDeveloper think it should be resolved here).

[fedarko]

Basically, the gradient color bar extends beyond the initial size of the legend which triggers the scroll bar. I would like to have both the legend title and gradient color bar visible without triggering the scroll bar but there is some odd css that I can't figure out.

I think I found out a way to (sort of) fix this problem, but I'm not yet sure why it works. Could you try removing overflow: auto; from the #legend-main CSS? It looks like this fixes the problem for me in Firefox (without this fix, I get the same problem in Firefox).

I am really not sure why the overflow thing breaks it in Firefox, since this same setting (overflow auto) is also used for the barplot layer legends, and those look fine in Firefox. I thought there was something weird going on with the resize CSS setting that was used for #legend-main, but removing resize entirely (and just keeping the overflow: auto thing) doesn't fix the problem -- only removing the overflow auto thing seems to fix it.

My best guess right now is that something about the overflow setting (in combination with the other CSS in effect on the main legend element) results in Firefox thinking that a scrollbar should be present, which maybe somehow messes with the SVG / element dimensions.

Strangely, this fix has the bizarre side effect (...just in Chrome, as far as I can tell) of making the default width of a continuous legend too wide. Although I guess this is a better problem to have :)

Notably-- removing overflow: auto will only be useful for the main legend when a continuous scale is being used. This is because we rely on overflow (and the ability to scroll stuff) when displaying categorical legends with a bunch of values -- without overflow these'll go off the legend. If we wind up going with this as a solution, I think we'd need to add/remove the overflow: auto CSS thing when changing the legend type somehow.

[kwcantrell]

I simplified this PR a bit. Please look at updated PR description.

[fedarko]

Thanks @kwcantrell! This is looking good.

[ElDeveloper]

@kwcantrell looks like the js style checker failed. Just flagging for your visibility.

[emperor-helper]

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv

[kwcantrell]

Thanks @fedarko and @ElDeveloper! I think I have addressed all of your comments so this pr should be good to merge.

[ElDeveloper]

Just one comment/question. Thanks so much @kwcantrell

[fedarko]

@kwcantrell I just went over the changes. It looks like the only two TODOs left are the non-numeric warning and testing continousFailedFunc, and both of those could be deferred to separate issues if you'd like. Let me know if you'd like me to just merge this as is, and I can do that / open issues as needed.

[kwcantrell]

@fedarko I updated the message for the non-numeric warning message for feature metadata coloring and added some test.

I fixed the bug were the gradient bar would overflow the legend. I changed containerSVG.setAttribute("height", "100%"); to containerSVG.setAttribute("height", "80%"); in addContinousKey in legend.js

[fedarko]

I have a few suggestions for fixes. I think once all of these are in we can merge this in. Thanks so much @kwcantrell!

[fedarko]

This works -- and I'm OK with merging it in as is, as long as the other changes I've suggested here are addressed -- but I think we could probably structure this code more clearly. To me, it is confusing that the Legend defaults to the barplot messages, and that these messages have to be passed in from Empress object (but the barplot messages are already located here).

I propose we restructure this so that the Legend class (on creation) takes as input an optional legendUse parameter of tree or barplot, which then adjusts what the full and short missing non-numeric warnings are (so all of the warning text will live in legend.js). That said: I'm totally OK with deferring this to a future issue so we can get this PR in soon.

[kwcantrell]

I agree, this was a fairly lazy solution. The goal was to modify the existing code as little as possible hence why it defaults to the barplot messages.

[kwcantrell]

Instead of adding a special parameter, I will make Legend an abstract class and add SampleFeatureColoringLegend and BarplotLegend that will inherit it. This will also make it a lot easier to change the behavior of the different legend types.

[fedarko]

One more note sorry --

I fixed the bug were the gradient bar would overflow the legend. I changed containerSVG.setAttribute("height", "100%"); to containerSVG.setAttribute("height", "80%"); in addContinousKey in legend.js

This works, thank you! However, the bug still seems to be a problem (albeit much less noticeable) when the warning message is shown:

It's def not something we gotta fix right now (the 80% solution should work great for most use cases!), but I just wanted to bring this up so that we know where to start when eventually working on #515.

[kwcantrell]

I fixed the legend gradient bar issue by placing the gradient svg within a div container. It now will be initialized as 80% of the legend size but will not be resized if the user resizes the legend.

I also abstracted the legend class and added SampleFeatureColorLegend and BarplotLegend.

[fedarko]

Changes look good -- the suggestions here are only minor things. The warning messages / legend style fixes are dope.

However, I found two bugs with this PR that we need to fix before merging. Will comment with that.

[fedarko]

Looks good.

Did some more testing of this. Found two bugs we need to fix:

1. Turning feature metadata coloring off and on again

Although the Color Map is reset when turning off feature metadata coloring, the Continuous values? checkbox isn't. This makes it possible for the user to try to trigger continuous coloring using the default categorical scale, which throws an error:

colorer.js:366 Uncaught Error: No gradient data defined for this Colorer; check that useQuantScale is true and that the selected color map is not discrete.
    at Colorer.getGradientInfo (colorer.js:366)
    at Empress.colorByFeatureMetadata (empress.js:2521)
    at SidePanel._colorFeatureTree (side-panel-handler.js:345)
    at SidePanel._updateColoring (side-panel-handler.js:304)
    at HTMLButtonElement.fUpdateBtn.onclick (side-panel-handler.js:641)

GIF of this happening:

There are probs multiple ways to fix this, but I think one of the easiest might be resetting Continuous values? to unchecked and hiding it whenever Feature Metadata Coloring is turned off.

2. Nodes left as the "default color" in the gradient are not assigned a proper color, and are left out of the tree export

This is a weird one. I was playing around with the tree exporting, and noticed that the tree nodes corresponding to non-numeric values seemed to be invisible in Chromium's SVG viewer:

In Inkscape, however, these same nodes seemed visible (same region, same SVG file):

It turns out that these nodes are getting assigned an invalid color, and I guess different SVG renderers will interpret this invalid color arbitrarily. I'm copying in one of the lines from the SVG file:

<line x1="1923.2021484375" y1="564.703125" x2="1928.596923828125" y2="566.2871704101562" stroke="rgb(NaN,NaN,NaN)"  />

Furthermore, I think this problem isn't just with the tree exporting -- these nodes actually aren't getting assigned colors, and we've been lucky in that this lack of color somehow gets turned into rgb(0, 0, 0) (which differs from the actual default color of rgb(64, 64, 64)). I don't know if it's easy to tell, but notice how a node with missing data actually subtly changes color when enabling continuous feature metadata coloring in the GIF below, even though it should remain the default color: this is because it's getting changed from the default dark gray color to black (rgb(0, 0, 0)).

I believe we need to fix this problem by adding another parameter to the Colorer class constructor, that indicates whether or not to include color info for missing values in the getMapRGB / getMapHex functions (and if so, what that default color should be). We could probably handle this in a single optional parameter named nonNumericColor or something (which should default to null or undefined). If this parameter is specified (could be a hex string or an RGB integer, whatever's easiest -- when supplied to Colorer by Empress, it should be directly derived from Empress.DEFAULT_COLOR) and gradient coloring is used, then all non-numeric values should be added (with this color) in scope.__valueToColor here:

empress/empress/support_files/js/colorer.js

Lines 260 to 265 in df3a4da

	_.each(split.numeric, function (n) {
	scope.__valueToColor[n] = interpolator(parseFloat(n));
	});
	// Figure out if we should show a warning message about missing /
	// non-numeric values to the user
	this._missingNonNumerics = split.nonNumeric.length > 0;

The code should be extendable with something like:

if (this._missingNonNumerics) {
    // (this next line will depend on if nonNumericColor is a hex
    // string, RGB tuple, RGB integer, etc.)
    var nonNumericColorChroma = chroma(nonNumericColor);
    _.each(split.nonNumeric, function (nn) {
        scope.__valueToColor[nn] = nonNumericColorChroma;
    });
}

We should test this, also (I think extending the current test-colorer stuff to handle this extra parameter should be feasible).

I guess the bright side is that having this parameter in here will make it easier in the future to let users change the color of missing / non-numeric barplots -- so there's that.

[ElDeveloper]

@kwcantrell can you resolve the conflicts?

[kwcantrell]

Furthermore, I think this problem isn't just with the tree exporting -- these nodes actually aren't getting assigned colors, and we've been lucky in that this lack of color somehow gets turned into rgb(0, 0, 0) (which differs from the actual default color of rgb(64, 64, 64)).

Good catch! I like the idea of adding a parameter to let users choose what color to assign non-numeric values in the case of continuous coloring. However, that would be a bit out of scope for this pr. So to fix the 2nd bug you found, I choose to simply remove non-numeric values from obs in empress.colorByFeateureMetadata. You solution does work however, it would lead to empress thickening the non-numeric lines (which I believe should not happen). But I think we should create an issue for choosing the non-numeric and link your comment since ultimately it would be a better solution.

[ElDeveloper]

@fedarko would you be able to look at @kwcantrell's answer to your comment? What else do we need to get this branch merged?

[ElDeveloper]

@kwcantrell, I was able to confirm that the issues that @fedarko pointed out were no longer there (missing data branches are assigned a 64,64,64 color value during export, and the exception is no longer raised. However, I did find a minor glitch related to the continuous values checkbox. It seems like the box is leftover after the menu is reset when the feature-metadata coloring is turned off. Once you click that while a non-continuous colorscheme is selected the checkbox disappears. After the checkbox disappears everything continues to work well. See this animated GIF:

---

Just in case, this is the code I ran to generate an additional 2 columns with continuous metadata:

import qiime2 as q2, pandas as pd, numpy as np
df = q2.Artifact.load('docs/moving-pictures/taxonomy.qza').view(pd.DataFrame)

# continuous category
df['seven_little_numbers'] = np.random.randint(0, 7, len(df))

# continuous category with missing values
df['six_little_numbers'] = df['seven_little_numbers'].copy()
df.loc[df.six_little_numbers == 0, 'six_little_numbers'] = 'I am not a number, I am a free man'

df.to_csv('docs/moving-pictures/feature-metadata.tsv', sep='\t')

qiime empress community-plot \
	--i-tree docs/moving-pictures/rooted-tree.qza \
	--i-feature-table docs/moving-pictures/table.qza \
	--m-sample-metadata-file docs/moving-pictures/sample_metadata.tsv \
	--m-feature-metadata-file docs/moving-pictures/feature-metadata.tsv \
	--o-visualization docs/moving-pictures/empress-tree.qzv