Add midpoint attr to color scales #3549

nicolaskruchten · 2019-02-15T19:31:20Z

LMK what kinds of extra tests you'd like or if you want me to just modify an existing mock or something, or if I've mangled some of the coercion machinery :)

nicolaskruchten · 2019-02-15T19:40:34Z

I've love to get this into 1.45 if it's uncontroversial :)

etpinard · 2019-02-15T20:14:00Z

I'm still 100% convinced cmid is the right name for this attribute. I'd like make this attribute consistent with a future axis attribute coerced when we have rangemode: 'symmetric'. Maybe rangemid would be better?

etpinard · 2019-02-15T20:14:52Z

At present rangemode: 'symmetric' means an axis range centered around 0, but we could make that more general in the future.

nicolaskruchten · 2019-02-15T20:15:31Z

I think we should stick with the current naming convention for right now, and when we do something range-related in the future we can carry mid along with it.

etpinard · 2019-02-15T20:16:05Z

the current naming convention

which is?

nicolaskruchten · 2019-02-15T20:16:26Z

marker.rangemid is really... unintuitive, in the same way as marker.showscale is... what range? what scale?

nicolaskruchten · 2019-02-15T20:16:50Z

the current naming convention
which is?

cmin, cmax, cmid ...?

etpinard · 2019-02-15T20:17:43Z

ok, but we're planning on changing cmin / cmax for crange

Does anyone in @plotly/plotly_js have an opinion about this?

nicolaskruchten · 2019-02-15T20:18:39Z

In future when we have layout.coloraxis I think we should use axis-like terminology like range etc for sure but I feel cmid fits in nicely with what's here atm.

That said, if rangemid is what it takes to get this merged in, I can change it. Might be odd in the histogram2d case where it's zmin/zmax at the trace level tho. trace.rangemid ?

nicolaskruchten · 2019-02-15T20:19:06Z

ok, but we're planning on changing cmin / cmax for crange

And the day we do that, we should change cmid also, is what I'm saying.

etpinard · 2019-02-15T20:22:14Z

Making changes to attributes is never fun. This is exactly what I'm trying to avoid.

nicolaskruchten · 2019-02-15T20:23:35Z

What's the rationale for cmin/cmax -> crange ?

etpinard · 2019-02-15T20:27:24Z

What's the rationale for cmin/cmax -> crange ?

Bring color "axis" attribute closer to ~~data~~ coordinate axis attributes.

nicolaskruchten · 2019-02-15T20:35:25Z

Bring color "axis" attribute closer to data axis attributes.

I would leave the existing cmin/cmax attributes alone (and add cmin!), and then use the range terminology only in the future layout.coloraxis object. Then in V2 we can drop the trace-specific colorscale/colorbar stuff.

etpinard · 2019-02-15T20:43:34Z

I would leave the existing cmin/cmax attributes alone (and add cmid)!

If you really want to merge this ASAP, sure what can that!

Now, are you planning on fixing the tests (we should also add jasmine tests making sure restyle and react work ok in and out of cmid) or you want me to take this thing to the finish line?

nicolaskruchten · 2019-02-15T20:52:21Z

I can finish it up, just wanted to get feedback on the approach, make sure I didn't miss anything obvious.

nicolaskruchten · 2019-02-15T20:53:07Z

If you really want to merge this ASAP, sure what can that!

Mostly as you say it's a pain to change existing attrs, so I think we should plan to leave the existing cmin/cmax alone for that reason.

nicolaskruchten · 2019-02-16T13:11:36Z

So I'm trying to fix the tests and confused about why the scattergeo one is failing but there is scattermapbox code in the stack trace. I'm not sure why execution is going down that path... any hints?

etpinard · 2019-02-18T16:53:55Z

So I'm trying to fix the tests and confused about why the scattergeo one is failing

plotly.js/src/plots/geo/layout/layout_attributes.js

Lines 96 to 102 in 0714286

    
           projection: { 
        
               type: { 
        
                   valType: 'enumerated', 
        
                   role: 'info', 
        
                   values: Object.keys(constants.projNames), 
        
                   description: 'Sets the projection type.' 
        
               },

Thanks for uncovering a pretty embarrassing typo. Switching to scattergeo makes the scattergeo_test.js suite pass.

Now, for scattermapbox, you'll need to add cmid to

plotly.js/src/traces/scattermapbox/attributes.js

Lines 73 to 99 in 0714286

    
           marker: { 
        
               symbol: { 
        
                   valType: 'string', 
        
                   dflt: 'circle', 
        
                   role: 'style', 
        
                   arrayOk: true, 
        
                   description: [ 
        
                       'Sets the marker symbol.', 
        
                       'Full list: https://www.mapbox.com/maki-icons/', 
        
                       'Note that the array `marker.color` and `marker.size`', 
        
                       'are only available for *circle* symbols.' 
        
                   ].join(' ') 
        
               }, 
        
               opacity: markerAttrs.opacity, 
        
               size: markerAttrs.size, 
        
               sizeref: markerAttrs.sizeref, 
        
               sizemin: markerAttrs.sizemin, 
        
               sizemode: markerAttrs.sizemode, 
        
               color: markerAttrs.color, 
        
               colorscale: markerAttrs.colorscale, 
        
               cauto: markerAttrs.cauto, 
        
               cmax: markerAttrs.cmax, 
        
               cmin: markerAttrs.cmin, 
        
               autocolorscale: markerAttrs.autocolorscale, 
        
               reversescale: markerAttrs.reversescale, 
        
               showscale: markerAttrs.showscale, 
        
               colorbar: colorbarAttrs,

as scattermapbox doesn't use the colorscale attribute generator (yet).

nicolaskruchten · 2019-02-18T21:53:21Z

Thanks for looking into it @etpinard ! I'm confused about what the "typo" is in the code you linked above... can you clarify please?

etpinard · 2019-02-18T21:56:07Z

I'm confused about what the "typo" is in the code you linked above... can you clarify please?

Oops. Wrong link:

plotly.js/test/jasmine/tests/scattergeo_test.js

Line 93 in 25fa0c2

var base = { type: 'scattermapbox' };

nicolaskruchten · 2019-02-18T22:00:55Z

Ahhh that makes much more sense, thank you :)

nicolaskruchten · 2019-02-19T14:37:39Z

OK despite best intentions I havent found the time to write tests here unfortunately... could someone in @plotly/plotly_js take over the restyle/relayout tests plz? :)

etpinard · 2019-02-19T15:08:16Z

Ok. I'll do that this morning.

nicolaskruchten · 2019-02-19T15:17:09Z

src/components/colorscale/attributes.js

+        valType: 'number',
+        role: 'info',
+        dflt: null,
+        editType: editTypeOverride || 'plot',


@etpinard I wasn't sure what to put for editType and impliedEdits btw so I copied over the settings from max above... maybe it should match auto instead? Or be something else? Likely your tests will expose any problems but I just wanted to shine a light here.

Restyle got fixed in 23c1cf0

Is that the behaviour you had in mind?

I must admit I don't fully grok the various nuances of the types of edits, but the test reads fine at first glance!

nicolaskruchten · 2019-02-19T17:28:08Z

test/jasmine/tests/plot_api_test.js

@@ -1195,6 +1195,77 @@ describe('Test plot api', function() {
            .then(done);
        });

+        it('turns on cauto when cmid is edited', function(done) {


this is an odd description... cmid only has effect if cauto is true, but doesn't turn on cauto per se, right?

but doesn't turn on cauto per se, right?

yes, it has to. If not, restyle(gd, 'marker.cmid', 0) wouldn't work.

by "not work" here I mean it wouldn't apply that new cmid value when cmin and cmax are set.

OK I see what you mean.

nicolaskruchten · 2019-02-19T17:28:58Z

src/components/colorscale/defaults.js

+    if(auto) {
+        coerce(prefix + cLetter + 'mid');
+    } else {
+        coerce(prefix + cLetter + 'min');


this line and the next used to always run, but now only conditionally... might this break anything? I'm not sure about the implication of not calling coerce().

No tests are failing because of it.

There is a test failure that mentions coerce:

Test surface supplyDefaults should coerce 'c' attributes with 'c' values regardless of `'z' if 'c' is present FAILED

Ah good eye.

Well cmin and cmax have no effect

plotly.js/src/components/colorscale/calc.js

Lines 31 to 37 in bb1239a

if(auto !== false || min === undefined) {

min = Lib.aggNums(Math.min, null, vals);

}

if(auto !== false || max === undefined) {

max = Lib.aggNums(Math.max, null, vals);

}

when cauto is true.

I'm easy. Like I said, I don't know the impact of calling coerce and ignoring the attr vs not calling coerce :)

Calling the behaviour correct and adapting the test in ff67812

etpinard · 2019-02-19T18:54:42Z

@plotly/plotly_js can I get a review for this thing?

archmoj · 2019-02-19T18:57:03Z

@plotly/plotly_js can I get a review for this thing?

@etpinard - sure, I'll review this.

archmoj

Great PR.
I was a bit confused when I was trying to paint a surface elevation using cmid and cmin.
Here a codepen.
I thought as described #3509 (comment) the user may try to declare the minimum and middle range. However; it lookscmin has no effect in the mentioned example when cmid is defined?

etpinard · 2019-02-19T21:29:22Z

However; it lookscmin has no effect in the mentioned example when cmid is defined?

Right, that's the expected behaviour it seems like:

plotly.js/src/components/colorscale/attributes.js

Lines 171 to 177 in ff67812

    
           description: [ 
        
               'Sets the mid-point of the color domain by scaling ', minFull, 
        
               ' and/or ', maxFull, ' to be equidistant to this point.', 
        
               effectDesc, 
        
               ' Value should have the same units as ', colorAttrFull, '. ', 
        
               'Has no effect when ', autoFull, ' is `false`.' 
        
           ].join('')

archmoj · 2019-02-19T21:37:33Z

Right, that's the expected behaviour it seems like:

Then it may be reasonable to modify the description of cmin and cmax? to include for example:

'Has no effect when `cmid` is defined.'

.

etpinard · 2019-02-19T21:40:48Z

Then it may be reasonable to modify the description of cmin and cmax? to include for example:

'Has no effect when cmid is defined.'

That's not true. Say you have:

{
  cmin: 0,
  cmax: 2,
  cmid: 2
}

then here cmid is defined, but ignored.

archmoj · 2019-02-19T21:54:45Z

Nicely done.
💃

nicolaskruchten · 2019-02-19T21:55:48Z

Yes the idea is that if you want to specify two points on the curve they must be min/max... not min/mid or mid/max. Easier to implement and document. The mid doc is a bit indirect in that it says “no effect if auto false” and then the auto doc says “false if min and max are set” but I figure this is still correct/readable.

Add midpoint attr to color scales

d69f27c

etpinard added feature something new status: in progress labels Feb 15, 2019

etpinard added this to the v1.45.0 milestone Feb 15, 2019

nicolaskruchten added 2 commits February 18, 2019 16:58

Merge branch 'master' into cmid

e2745f8

fixing tests

b8dce0a

removing unnecessary mock

ba72887

nicolaskruchten commented Feb 19, 2019

View reviewed changes

etpinard added 2 commits February 19, 2019 12:07

fix and 🔒 cmid/zmid edits

23c1cf0

add cmid/zmid mock

40a7510

nicolaskruchten commented Feb 19, 2019

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Feb 19, 2019

fix surface cmin/cmax/cauto defaults test

ff67812

archmoj self-requested a review February 19, 2019 18:57

archmoj reviewed Feb 19, 2019

View reviewed changes

etpinard merged commit 2e7f533 into master Feb 19, 2019

etpinard deleted the cmid branch February 19, 2019 21:56

etpinard mentioned this pull request Feb 19, 2019

autocolorscale doesn't seem to pickup on diverging numberlines #3064

Closed

cpsievert mentioned this pull request Mar 11, 2019

Add an example of a diverging colorscale cpsievert/plotly_book#10

Open

nicolaskruchten mentioned this pull request Apr 29, 2019

Shared colorscales/colorbars aka coloraxes #3431

Closed

	if(auto !== false \|\| min === undefined) {
	min = Lib.aggNums(Math.min, null, vals);
	}

	if(auto !== false \|\| max === undefined) {
	max = Lib.aggNums(Math.max, null, vals);
	}

Add midpoint attr to color scales #3549

Add midpoint attr to color scales #3549

Conversation

nicolaskruchten commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

etpinard commented Feb 15, 2019 • edited Loading

etpinard commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

etpinard commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

etpinard commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

etpinard commented Feb 15, 2019 • edited Loading

nicolaskruchten commented Feb 15, 2019

etpinard commented Feb 15, 2019 • edited Loading

nicolaskruchten commented Feb 15, 2019

etpinard commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

nicolaskruchten commented Feb 15, 2019

nicolaskruchten commented Feb 16, 2019

etpinard commented Feb 18, 2019

nicolaskruchten commented Feb 18, 2019

etpinard commented Feb 18, 2019

nicolaskruchten commented Feb 18, 2019

nicolaskruchten commented Feb 19, 2019

etpinard commented Feb 19, 2019

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

etpinard Feb 19, 2019 • 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

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

etpinard commented Feb 19, 2019

archmoj commented Feb 19, 2019

archmoj left a comment

Choose a reason for hiding this comment

etpinard commented Feb 19, 2019

archmoj commented Feb 19, 2019 • edited Loading

etpinard commented Feb 19, 2019

archmoj commented Feb 19, 2019

nicolaskruchten commented Feb 19, 2019

etpinard commented Feb 15, 2019 •

edited

Loading

etpinard commented Feb 15, 2019 •

edited

Loading

etpinard commented Feb 15, 2019 •

edited

Loading

etpinard Feb 19, 2019 •

edited

Loading

archmoj commented Feb 19, 2019 •

edited

Loading