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

Add color mapping to the bar chart #1218

Merged
merged 6 commits into from
Mar 14, 2020

Conversation

mweiden
Copy link
Contributor

@mweiden mweiden commented Mar 12, 2020

Finishing up #1035

Changes made on top of @Donaldpherman's PR:

  • Only color histograms that are selected for colorby
  • Reuse binning functions from util/dataframe/histogram.js; this fixes an issue with d3 generating near-zero width bins
  • Fix/simplify histogram color mapping functions
  • Do not attempt to plot bins if the calculated binWidth is zero; this can happen if all dataframe column values are the same
  • Refactor the function that draws the histogram a bit

Illustration of fixes to color mapping are below. Each shows the legend scaled against the colored histogram bins.

Before (source @liaprins-czi):
Screen Shot 2020-03-11 at 11 32 26 AM

After (source this PR):
Screen Shot 2020-03-11 at 10 38 15 PM

@mweiden mweiden self-assigned this Mar 12, 2020
@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #1218 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1218   +/-   ##
=======================================
  Coverage   59.39%   59.39%           
=======================================
  Files          64       64           
  Lines        4692     4692           
  Branches      372      372           
=======================================
  Hits         2787     2787           
  Misses       1813     1813           
  Partials       92       92
Flag Coverage Δ
#backend 45.59% <ø> (ø) ⬆️
#frontend 74.92% <ø> (ø) ⬆️
#javascript 74.92% <ø> (ø) ⬆️
#python 45.59% <ø> (ø) ⬆️
#smokeTest ?
#smokeTestAnnotations 100% <ø> (ø) ⬆️
#unitTest 59.39% <ø> (ø) ⬆️

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 023a73c...eef9aff. Read the comment docs.

Copy link

@liaprins-czi liaprins-czi left a comment

Choose a reason for hiding this comment

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

Looks great to me; thanks Matt!

Donaldpherman and others added 5 commits March 12, 2020 20:01
Coloring of Histrogram based up the x axis instead of y axis.
* Only color histograms that are selected for colorby
* Add some small refactors to BrushableHistogram's componentDidUpdate
* Reuse binning functions from util/dataframe/histogram.js; this fixes
an issue with there being near-zero width bins
* Fix color mapping so that it matches the scale in the legend
* Do not attempt to plot bins if the calculated binWidth is zero; this
can happen if all values are the same
* Refactor the function that draws the histogram a bit
@mweiden mweiden force-pushed the donaldpherman/color-mapping-for-bar-chart branch from eef9aff to 8b083e8 Compare March 13, 2020 03:01
@codecov-io
Copy link

codecov-io commented Mar 14, 2020

Codecov Report

Merging #1218 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1218   +/-   ##
=======================================
  Coverage   59.40%   59.40%           
=======================================
  Files          64       64           
  Lines        4707     4707           
  Branches      374      374           
=======================================
  Hits         2796     2796           
  Misses       1819     1819           
  Partials       92       92           
Flag Coverage Δ
#backend 45.52% <ø> (ø)
#frontend 75.00% <ø> (ø)
#javascript 75.00% <ø> (ø)
#python 45.52% <ø> (ø)
#smokeTest ?
#smokeTestAnnotations 100.00% <ø> (ø)
#unitTest 59.40% <ø> (ø)

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 f28bd52...cfd1ebd. 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.

Brief testing shows that this PR has introduced a bug with brush display in histograms.

Test case:

  1. load a dataset (I used TM_facs, and will refer to it throughout - although the bug should manifest in any dataset)
  2. brush select a continuous selection (I did n_counts):
    image

Note that the main graph shows the subset of selected cells.

  1. Click color-by (droplet) button on same histogram. Note that the brush selection is gone, but that the cells are (correctly) still selected in the main graph.
    image

The cells should stay selected (color-by changes do not modify the selection). What is very likely happening is that the changes to componentDidUpdate() fail to correctly recognize when the brush needs to be re-rendered.

I have noted the line where this bug is located.

@mweiden mweiden force-pushed the donaldpherman/color-mapping-for-bar-chart branch from cfd1ebd to 9bffe3d Compare March 14, 2020 00:43
@mweiden mweiden requested a review from bkmartinjr March 14, 2020 03:27
@mweiden mweiden merged commit 86cbe64 into master Mar 14, 2020
@mweiden mweiden deleted the donaldpherman/color-mapping-for-bar-chart branch March 14, 2020 22:24
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.

None yet

5 participants