Skip to content
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

If min / max are the same, hide histo + legend, show value #1554

Merged
merged 6 commits into from
Jun 11, 2020

Conversation

colinmegill
Copy link
Contributor

Fixes #1497

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #1554 into master will increase coverage by 5.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1554      +/-   ##
==========================================
+ Coverage   61.74%   66.98%   +5.24%     
==========================================
  Files          36       72      +36     
  Lines        3568     5876    +2308     
  Branches        0      382     +382     
==========================================
+ Hits         2203     3936    +1733     
- Misses       1365     1849     +484     
- Partials        0       91      +91     
Flag Coverage Δ
#backend 61.74% <ø> (ø)
#frontend 75.08% <ø> (?)
#javascript 75.08% <ø> (?)
#python 61.74% <ø> (ø)
#unitTest 66.98% <ø> (+5.24%) ⬆️
Impacted Files Coverage Δ
...ent/__tests__/util/stateManager/sampleResponses.js 100.00% <0.00%> (ø)
client/src/reducers/undoable.js 70.23% <0.00%> (ø)
client/src/util/fromEntries.js 100.00% <0.00%> (ø)
client/src/util/stateManager/colorHelpers.js 2.66% <0.00%> (ø)
client/src/globals.js 92.00% <0.00%> (ø)
client/src/components/framework/toasters.js 54.54% <0.00%> (ø)
client/src/util/finiteExtent.js 0.00% <0.00%> (ø)
client/src/util/typedCrossfilter/bitArray.js 77.77% <0.00%> (ø)
client/src/util/typedCrossfilter/sort.js 89.39% <0.00%> (ø)
client/src/util/stateManager/annotationsHelpers.js 3.94% <0.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f063708...cee4b67. Read the comment docs.

Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unclear why the toggle between single-value and multi-value (histogram) is based upon non-clipped values rather than clipped. Seems like clipped is the right range to use, as it more reliably tells you if the histogram is single-valued.

@@ -476,6 +476,8 @@ class HistogramBrush extends React.PureComponent {
const unclippedRangeMaxColor =
world.clipQuantiles.max === 1 ? "#bbb" : globals.blue;

const isSingleValue = unclippedRangeMin === unclippedRangeMax;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what behavior are you targeting when the user has clipped the range, so that it is all single-valued or NaN (clipped)? In this case, the NaN doesn't show up in the histogram, and the histogram will end up being single-valued.

So shouldn't the conditional be based upon the min/max of the clipped range, rather than the unclipped range?

@@ -115,7 +116,13 @@ class ContinuousLegend extends React.Component {
d3.select("#continuous_legend").selectAll("*").remove();
}

if (colorAccessor && colorScale && colorScale.range) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a lot more readable if it was something like:

const range = colorScale?.range;
const [domainMin, domainMax] = colorScale?.domain?.() ?? [0, 0];
if (colorAccessor && colorScale && range && domainMin < domainMax) {
  ...
}

and then use those variables further down

Copy link
Member

@seve seve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

client/src/components/continuousLegend/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@colinmegill colinmegill merged commit 77ffa07 into master Jun 11, 2020
@colinmegill colinmegill deleted the colinmegill/#1497-histo-zeros branch June 11, 2020 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gene expression not shown in gene's histogram after subset (not all genes, though)
4 participants