biocore / empress

A fast and scalable phylogenetic tree viewer for microbiome data analysis
BSD 3-Clause "New" or "Revised" License
48 stars 31 forks source link

Gradient Coloring Option for Feature Metadata #484

Open kwcantrell opened 3 years ago

kwcantrell commented 3 years ago

~~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). Screenshot from 2021-02-09 21-38-50 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 commented 3 years ago

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

whoops

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 commented 3 years ago

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

ElDeveloper commented 3 years ago

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

emperor-helper commented 3 years ago

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

kwcantrell commented 3 years ago

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

fedarko commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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:

Peek 2021-06-11 19-33

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 commented 3 years ago

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 commented 3 years ago

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:

gc

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:

image

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

image

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

subtle

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:

https://github.com/biocore/empress/blob/df3a4da5a5b23d465937112dfa2050f8957bb0ed/empress/support_files/js/colorer.js#L260-L265

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 commented 3 years ago

@kwcantrell can you resolve the conflicts?

kwcantrell commented 3 years ago

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 commented 3 years ago

@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 commented 2 years ago

@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:

pr


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