Improve categorical color management #5815

kristw · 2018-09-05T01:25:42Z

Rewrite how categorical color scales and color schemes are managed.

Create centralized singleton ColorSchemeManager for managing color schemes, with mechanism for registering schemes.
Separate color schemes into their own files.
Create color scale class CategoricalColorScale that can be passed around as an object and support color overriding (forcedColors).
Create notion of CategoricalColorNamespace which can contain one or more scales. Any color overriding (forcedColors) are scoped within the namespace. This is useful for dashboard. @graceguo-supercat - you can also add new function for serializing the namespace and use it for saving dashboard color config.
Color overriding now can be set at scale-level (e.g. only for scale that uses bnbColors) or namespace-level (i.e. all scales within the namespace).
Separate logic for forcing colors and getting colors.
Add shim for legacy function getColorFromScheme, which can be removed in follow-up PR.

More details please read the unit test output below which also serves as documentation.

Testing

Pass existing unit tests for color.
Add new unit tests for the new classes.

  CategoricalColorScale
    ✓ exists
    new CategoricalColorScale(colors, parentForcedColors)
      ✓ can create new scale when parentForcedColors is not given
      ✓ can create new scale when parentForcedColors is given
    .getColor(value)
      ✓ returns same color for same value
      ✓ returns different color for consecutive items
      ✓ recycles colors when number of items exceed available colors
    .setColor(value, forcedColor)
      ✓ overrides default color
      ✓ does not override parentForcedColors
      ✓ returns the scale
    .toFunction()
      ✓ returns a function that wraps getColor
  CategoricalColorNamespace
    ✓ The class constructor cannot be accessed directly
    static getNamespace()
      ✓ returns default namespace if name is not specified
      ✓ returns namespace with specified name
      ✓ returns existing instance if the name already exists
    .getScale()
      ✓ returns a CategoricalColorScale from given scheme name
      ✓ returns same scale if the scale with that name already exists in this namespace
    .setColor()
      ✓ overwrites color for all CategoricalColorScales in this namespace
      ✓ can override forcedColors in each scale
      ✓ does not affect scales in other namespaces
      ✓ returns the namespace instance
    static getScale()
      ✓ getScale() returns a CategoricalColorScale with default scheme in default namespace
      ✓ getScale(scheme) returns a CategoricalColorScale with specified scheme in default namespace
      ✓ getScale(scheme, namespace) returns a CategoricalColorScale with specified scheme in specified namespace
    static getColor()
      ✓ getColor(value) returns a color from default scheme in default namespace
      ✓ getColor(value, scheme) returns a color from specified scheme in default namespace
      ✓ getColor(value, scheme, namespace) returns a color from specified scheme in specified namespace
  ColorSchemeManager
    ✓ The class constructor cannot be accessed directly
    static getInstance()
      ✓ returns a singleton instance
    .getScheme()
      ✓ .getScheme() returns default color scheme
      ✓ .getScheme(name) returns color scheme with specified name
    .getAllSchemes()
      ✓ returns all registered schemes
    .getDefaultSchemeName()
      ✓ returns default scheme name
    .setDefaultSchemeName()
      ✓ set default scheme name
      ✓ returns the ColorSchemeManager instance
    .registerScheme(name, colors)
      ✓ sets schemename and color
      ✓ returns the ColorSchemeManager instance
    .registerMultipleSchemes(object)
      ✓ sets multiple schemes at once
      ✓ returns the ColorSchemeManager instance
    static getScheme()
      ✓ is equivalent to getInstance().getScheme()
    static getAllSchemes()
      ✓ is equivalent to getInstance().getAllSchemes()
    static getDefaultSchemeName()
      ✓ is equivalent to getInstance().getDefaultSchemeName()
    static setDefaultSchemeName()
      ✓ is equivalent to getInstance().setDefaultSchemeName()
    static registerScheme()
      ✓ is equivalent to getInstance().registerScheme()
    static registerMultipleSchemes()
      ✓ is equivalent to getInstance().registerMultipleSchemes()

codecov-io · 2018-09-05T01:43:42Z

Codecov Report

Merging #5815 into master will increase coverage by 0.09%.
The diff coverage is 73.03%.

@@            Coverage Diff             @@
##           master    #5815      +/-   ##
==========================================
+ Coverage   63.73%   63.83%   +0.09%     
==========================================
  Files         374      381       +7     
  Lines       23325    23446     +121     
  Branches     2607     2614       +7     
==========================================
+ Hits        14867    14966      +99     
- Misses       8445     8467      +22     
  Partials       13       13

Impacted Files	Coverage Δ
superset/assets/src/visualizations/chord.jsx	`0% <0%> (ø)`	⬆️
superset/assets/src/common.js	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/partition.js	`0% <0%> (ø)`	⬆️
...t/assets/src/dashboard/reducers/getInitialState.js	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/sunburst.js	`0% <0%> (ø)`	⬆️
...t/assets/src/visualizations/wordcloud/WordCloud.js	`0% <0%> (ø)`	⬆️
...sualizations/deckgl/CategoricalDeckGLContainer.jsx	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/rose.js	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/treemap.js	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/sankey.js	`0% <0%> (ø)`	⬆️
... and 18 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 c82cea3...1cf4695. Read the comment docs.

williaster

This looks really great overall! thanks for improving the color model significantly 🥇

Left a couple of pretty minor comments.

williaster · 2018-09-11T19:19:49Z

superset/assets/src/modules/CategoricalColorScale.js

+export default class CategoricalColorScale {
+  constructor(colors, sharedForcedColors) {
+    this.colors = colors;
+    this.sharedForcedColors = sharedForcedColors;


could you add a comment that clarifies the distinction between sharedForcedColors and forcedColors?

from reading it seems like

sharedForcedColors takes precedence over forcedColos if present and

sharedForcedColors is never modified vs forcedColors which is

sharedForcedColors is an optional parameter that comes from parent (usually CategoricalColorNamespace) and supersede this.forcedColors. This let us do the namespace-level color override across all scales.

I added more comments in the code.

williaster · 2018-09-11T19:20:36Z

superset/assets/src/modules/CategoricalColorScale.js

@@ -0,0 +1,58 @@
+import { TIME_SHIFT_PATTERN } from '../utils/common';
+
+export function cleanValue(value) {


could be in it's own file but don't feel strongly

I agree it should not be here at all but have to track down which component really need this cleaning. can be tricky

This might be mostly nvd3 so let's keep it here for now and I will get rid of it in a follow-up PR.

williaster · 2018-09-11T19:21:05Z

superset/assets/src/modules/CategoricalColorNamespace.js

+}
+
+const namespaces = {};
+const DEFAULT_NAMESPACE = 'GLOBAL';


any use in exporting this so you could import it for tests? and/or adding color somewhere in the value for easier debugging.

superset/assets/spec/javascripts/modules/CategoricalColorScale_spec.js

williaster · 2018-09-11T19:23:23Z

superset/assets/spec/javascripts/modules/CategoricalColorScale_spec.js

+      expect(c1).to.equal(c3);
+      expect(c2).to.equal(c5);
+    });
+    it('returns different color for consecutive items', () => {


do you think it's worthwhile to test color value recycling logic if # items > # colors?

Great idea. Just add another test for this.

williaster · 2018-09-11T20:08:50Z

superset/assets/src/modules/ColorSchemeManager.js

+
+// These registration code eventually should go into per-app configuration
+// when we migrate to the plug-in system.
+getInstance()


would it be worth moving it to the appSetup call in common.js?

kristw · 2018-09-12T06:16:56Z

Addressed comments and rename sharedForceColors to parentForcedColors

williaster

nice! this LGTM! 🌈

kristw · 2018-09-12T19:23:13Z

Thank you. Ready to merge when you are. Best regards, Krist

…

-- Krist Wongsuphasawat http://kristw.yellowpigz.com

On Sep 12, 2018, 11:18 -0700, Chris Williams ***@***.***>, wrote: @williaster approved this pull request. nice! this LGTM! 🌈 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

* Create new classes for handling categorical colors * verify to pass existing unit tests * separate logic for forcing color and getting color * replace getColorFromScheme with CategoricalColorManager * organize static functions * migrate to new function * Remove ALL_COLOR_SCHEMES * move sequential colors to another file * extract categorical colors to separate file * move airbnb and lyft colors to separate files * fix missing toFunction() * Rewrite to support local and global force items, plus namespacing. * fix references * revert nvd3 * update namespace api * Update the visualizations * update usage with static functions * update unit test * add unit test * rename default namespace * add unit test for color namespace * add unit test for namespace * start unit test for colorschememanager * add unit tests for color scheme manager * check returns for chaining * complete unit test for the new classes * fix color tests * update unit tests * update unit tests * move color scheme registration to common * update unit test * rename sharedForcedColors to parentForcedColors * remove import

kristw force-pushed the kristw-colors branch from e40ed82 to 30d4bfe Compare September 8, 2018 00:31

kristw changed the title ~~Improve categorical color management~~ [WIP] Improve categorical color management Sep 8, 2018

kristw force-pushed the kristw-colors branch from 3136d7e to 3e8772f Compare September 10, 2018 22:52

kristw changed the title ~~[WIP] Improve categorical color management~~ Improve categorical color management Sep 11, 2018

williaster reviewed Sep 11, 2018

View reviewed changes

kristw added 24 commits September 11, 2018 21:50

Create new classes for handling categorical colors

3c1210c

verify to pass existing unit tests

385a21a

separate logic for forcing color and getting color

1502fcb

replace getColorFromScheme with CategoricalColorManager

1fc211d

organize static functions

e3ddbd5

migrate to new function

dd60dc7

Remove ALL_COLOR_SCHEMES

211a777

move sequential colors to another file

c1aeeb5

extract categorical colors to separate file

56f9ef3

move airbnb and lyft colors to separate files

f3671c8

fix missing toFunction()

9894368

Rewrite to support local and global force items, plus namespacing.

943ecac

fix references

3300080

revert nvd3

2efac2e

update namespace api

1515f05

Update the visualizations

cd48bd6

update usage with static functions

2915afc

update unit test

d60d4ee

add unit test

f456065

rename default namespace

75318f4

add unit test for color namespace

9d457a0

add unit test for namespace

00d8090

start unit test for colorschememanager

8a91c4d

add unit tests for color scheme manager

47c5f20

kristw added 8 commits September 11, 2018 21:50

check returns for chaining

e2dba45

complete unit test for the new classes

500f441

fix color tests

1312dd5

update unit tests

9dbccd6

update unit tests

d08be39

move color scheme registration to common

06970de

update unit test

527e244

rename sharedForcedColors to parentForcedColors

1cb5d61

kristw force-pushed the kristw-colors branch from 20f145e to 1cb5d61 Compare September 12, 2018 04:59

remove import

1cf4695

williaster approved these changes Sep 12, 2018

View reviewed changes

williaster merged commit f482a6c into apache:master Sep 12, 2018

kristw deleted the kristw-colors branch September 12, 2018 21:17

graceguo-supercat mentioned this pull request Sep 13, 2018

[SIP-8] Add UI component for user to select and persist category color from Dashboard #5887

Closed

3 tasks

mistercrunch mentioned this pull request Sep 13, 2018

Empty "Color Scheme" dropdown #5889

Closed

kristw mentioned this pull request Sep 20, 2018

Deprecate getColorFromScheme #5937

Merged

kristw mentioned this pull request Oct 19, 2018

[Refactor] Extend color scheme management to sequential schemes #6150

Merged

junlincc mentioned this pull request Dec 15, 2020

[chart]:categorical color scales and color schemes is broken on treemap #12068

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve categorical color management #5815

Improve categorical color management #5815

kristw commented Sep 5, 2018 •

edited

Loading

codecov-io commented Sep 5, 2018 •

edited

Loading

williaster left a comment

williaster Sep 11, 2018

kristw Sep 11, 2018

kristw Sep 11, 2018

williaster Sep 11, 2018

kristw Sep 11, 2018

kristw Sep 12, 2018 •

edited

Loading

williaster Sep 11, 2018 •

edited

Loading

williaster Sep 11, 2018

kristw Sep 12, 2018

williaster Sep 11, 2018

kristw commented Sep 12, 2018

williaster left a comment

kristw commented Sep 12, 2018 via email

		@@ -0,0 +1,58 @@
		import { TIME_SHIFT_PATTERN } from '../utils/common';

		export function cleanValue(value) {

Improve categorical color management #5815

Improve categorical color management #5815

Conversation

kristw commented Sep 5, 2018 • edited Loading

Testing

codecov-io commented Sep 5, 2018 • edited Loading

Codecov Report

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

williaster Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw commented Sep 12, 2018

williaster left a comment

Choose a reason for hiding this comment

kristw commented Sep 12, 2018 via email

kristw commented Sep 5, 2018 •

edited

Loading

codecov-io commented Sep 5, 2018 •

edited

Loading

kristw Sep 12, 2018 •

edited

Loading

williaster Sep 11, 2018 •

edited

Loading