Skip to content

Colorspec doc corrections and clarifications #625

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Colorspec doc corrections and clarifications #625

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1fda97f
#581 colorspec corrections and clarifications
monfera Jun 10, 2016
a34c96a
#581 colorspec corrections and clarifications (PR feedback)
monfera Jun 10, 2016
cee831c
#581 colorspec corrections and clarifications
monfera Jun 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/components/colorscale/color_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

var colorScaleAttributes = require('./attributes');
var extendDeep = require('../../lib/extend').extendDeep;
var palettes = require('./scales.js');

module.exports = function makeColorScaleAttributes(context) {
return {
Expand All @@ -19,7 +20,7 @@ module.exports = function makeColorScaleAttributes(context) {
role: 'style',
description: [
'Sets the ', context, ' color. It accepts either a specific color',
' or an array of values that are mapped to the colorscale',
' or an array of numbers that are mapped to the colorscale',
Copy link
Contributor

Choose a reason for hiding this comment

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

Numbers or numeric strings to be more precise. Not sure if it's worth adding that amount of details in here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard I can cancel the PR and just make this single word change in the main PR. However to understand my motive, I started some work on the basis of what these docs say, and while their meanings are trivial to you due to developing these props in the first place, it was partly opaque to me, and in one case, misleading, so I had to resort to trial / error (at that point I intentionally didn't just look at the code). So I thought that if I as a developer had a problem understanding it then others may benefit from a modest rephrasing. So I'm glad to do what's the best fit for the system overall. Unrelated: I didn't quite understand the numeric string part as in JSON you'd pass on an array of plain numbers.

' relative to the max and min values of the array or relative to',
' `cmin` and `cmax` if set.'
].join('')
Expand All @@ -35,14 +36,19 @@ module.exports = function makeColorScaleAttributes(context) {
' values are required. For example,',
' `[[0, \'rgb(0,0,255)\', [1, \'rgb(255,0,0)\']]`.',
' To control the bounds of the colorscale in color space,',
' use `', context, '.cmin` and `', context, '.cmax`.'
].join('')
' use `', context, '.cmin` and `', context, '.cmax`.',
' Alternatively, `colorscale` may be a palette name string',
' of the following list: '
].join('').concat(Object.keys(palettes).join(', '))
}),
cauto: extendDeep({}, colorScaleAttributes.zauto, {
description: [
'Has an effect only if `', context, '.color` is set to a numerical array.',
' Determines the whether or not the color domain is computed',
' automatically.'
'Has an effect only if `', context, '.color` is set to a numerical array',
' and `cmin`, `cmax` are set by the user. In this case,',
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. cmin and cmax are auto-calc'ed if not provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard The term "set by the user" is being used to avoid this ambiguity (I wanted to stick to user visible / user controllable things in the doc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard in line with your above notes on the cmin/cmax auto-calculation, I think my text addition is legit: if the user does not set cmin/cmax then they'll be auto-calculated to the color array lo/hi values, IOW cauto will have no (visible) effect.

Sorry to be so tedious about the doc strings but I'm adding some test cases in general to reify the official API behaviors and since it's nontrivial stuff (and in some cases, by necessity, there are semiarbitrary choices), I feel the need to work against definitions that are clear to me. E.g. if you want to permit that only one of cmin/cmax is specified, I'd not only update the docs but also make tests that way so it's not purely about the doc strings.

' it controls whether the range of colors in `colorscale` is mapped to',
' the range of values in the `color` array (`cauto: true`), or the `cmin`/`cmax`',
' values (`cauto: false`).',
' Defaults to `false` when `cmin`, `cmax` are set by the user.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just clarification as the former text is explanatory only once the user is already familiar with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording in this is still a bit muddy - the In this case sentence in particular.

Can we not say (in some way) the cmin and cmax "map" the colorscale bounds to data or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdtusz I've updated it to

In this case, it controls whether the color range represented by `colorscale` is mapped to
the value bounds of the `color` array (`cauto: true`), or to the `cmin`..`cmax`
 value bounds (`cauto: false`).

Is it okay? I feel there's a tradeoff among using the best terminology (concept of mapping), comprehension by all ('what is mapping?') and brevity; maybe this version is at a better point in that space. (I haven't pushed it yet to avoid unnecessary CI testing)

Copy link
Contributor

Choose a reason for hiding this comment

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

Summoning Docs-Master @cldougl!

Copy link
Member

Choose a reason for hiding this comment

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

taking a 👀 now -- give me 1 min to catch up on the context

].join('')
}),
cmax: extendDeep({}, colorScaleAttributes.zmax, {
Expand All @@ -64,14 +70,18 @@ module.exports = function makeColorScaleAttributes(context) {
autocolorscale: extendDeep({}, colorScaleAttributes.autocolorscale, {
Copy link
Contributor Author

@monfera monfera Jun 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@etpinard Is it still true that both or neither of cmin,cmax must be set? My experience with scatter3d showed that omitting one of them did the reasonable thing: it behaved as if the omitted cm... value were set to the lowest (or highest) value of the color numeric array.

Copy link
Contributor

@etpinard etpinard Jun 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Correct. It is a little more subtle at the moment. See this line in the colorscale defaults.

If one of cmin or cmax isn't valid, then autoscale is defaulted to false. But, if the user set autoscale to true, the missing cmin or cmax gets auto-calc'ed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard yes, tests support you, assuming you actually meant cauto (autocolorscale just toggles between a default palette vs. a palette determined by the user-supplied colorscale).

In any case, our common conclusion is that the user does not need to set both of cmin and cmax, yet the current doc says both are needed. I'll leave it as it is because, with the current helptext, partial cmin / cmax specification leads to undefined results, which is of course fine even if we handle it gracefully. I just wasn't sure if you wanted to keep this constraint in the helpstring, or it was a case of just not having updated the doc string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

p.s. by the way, a reason for even considering partial cmin/cmax is that there's utility in clamping only one side of the domain (e.g. use the scale for water temperatures but all frozen, sub-zero points should be icy bue), which I presume is the reason for having it implemented resiliently against the case of partial bounds specification.

description: [
'Has an effect only if `', context, '.color` is set to a numerical array.',
' Determines whether or not the colorscale is picked using',
' values inside `', context, '.color`.'
' Determines whether the colorscale is a default palette (`autocolorscale: true`)',
' or the palette determined by `', context, '.colorscale`.',
' In case `colorscale` is unspecified or `autocolorscale` is true, the default ',
' palette will be chosen according to whether numbers in the `color` array are',
' all positive, all negative or mixed.'
].join('')
}),
reversescale: extendDeep({}, colorScaleAttributes.reversescale, {
description: [
'Has an effect only if `', context, '.color` is set to a numerical array.',
' Reverses the colorscale.'
' Reverses the color mapping if true (`cmin` will correspond to the last color',
' in the array and `cmax` will correspond to the first color).'
].join('')
})
};
Expand Down