-
Notifications
You must be signed in to change notification settings - Fork 0
189 ordinal scale domain item ordering #1
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
base: master
Are you sure you want to change the base?
Changes from 34 commits
87ab745
a547482
69bf832
3fd3cc0
e2977c5
62c4ac5
85d60d6
f6c40b6
e4f2167
03643fe
9c366ce
f626b08
2f939af
ce35e8b
262c747
5f33a7b
f4214cb
cbc98fd
a806ca2
5ad6b5c
82076f2
6fb2871
616121b
89794c6
849978e
d6fad10
7c355b8
8a2292c
9b01bcc
010451b
6c9d0b5
166aeb4
e472e14
d16fe6b
0314f37
043ac1b
b98bd65
e007f73
34a5054
c6d44d9
44a0c3d
613fda3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,9 @@ var Plots = require('../plots'); | |
var layoutAttributes = require('./layout_attributes'); | ||
var handleTickValueDefaults = require('./tick_value_defaults'); | ||
var handleTickDefaults = require('./tick_defaults'); | ||
var handleCategoryModeDefaults = require('./category_mode_defaults'); | ||
var setConvert = require('./set_convert'); | ||
var orderedCategories = require('./ordered_categories'); | ||
var cleanDatum = require('./clean_datum'); | ||
var axisIds = require('./axis_ids'); | ||
|
||
|
@@ -64,6 +66,10 @@ module.exports = function handleAxisDefaults(containerIn, containerOut, coerce, | |
} | ||
} | ||
|
||
containerOut._initialCategories = axType === 'category' ? | ||
orderedCategories(letter, containerIn.categorymode, containerIn.categorylist, options.data) : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll need to coerce Coercion is done in this scope by the shortcut coerce function defined in Moreover, we'll need to add logic around We may want to look up Don't hesitate to ask me any questions about our coercion framework. Surprising, it may the trickiest part to understand about plotly.js. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard Looking into this now as it appears to be the last piece in this CR. My test cases will need an update once coercion is in place, to switch to the more data-driven default or override behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard tl;dr blocking question for you at the end. As an aid to thinking about goals, here's a summary of what coercions I'm planning:
... well it looks like the two things you listed :-) My original take was that Taking it one step further, shouldn't we just have one property, e.g. called I'm now plunging into the basic coercion so we don't have an unexpected enumerative value but your input to the above question would be useful. It's not a big deal to unify the two properties and at least technically, it would be less ambiguous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
... and you would be correct. What smart coercion is trying to remove redundancy. For example, var layout: {
xaxis: {
categorylist: ['apples', 'bananas', 'clementines']
}
}; should be considered the same as: var layout: {
xaxis: {
categorymode: 'array', // redundant !!!
categorylist: ['apples', 'bananas', 'clementines']
}
}; But, var layout: {
xaxis: {
categorymode: 'category descending', // still used as a switch
categorylist: ['apples', 'bananas', 'clementines'] // not used in graph
}
}; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
[]; | ||
|
||
setConvert(containerOut); | ||
|
||
coerce('title', defaultTitle); | ||
|
@@ -91,6 +97,7 @@ module.exports = function handleAxisDefaults(containerIn, containerOut, coerce, | |
|
||
handleTickValueDefaults(containerIn, containerOut, coerce, axType); | ||
handleTickDefaults(containerIn, containerOut, coerce, axType, options); | ||
handleCategoryModeDefaults(containerIn, containerOut, coerce); | ||
|
||
var lineColor = Lib.coerce2(containerIn, containerOut, layoutAttributes, 'linecolor'), | ||
lineWidth = Lib.coerce2(containerIn, containerOut, layoutAttributes, 'linewidth'), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/** | ||
* Copyright 2012-2016, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
|
||
'use strict'; | ||
|
||
module.exports = function handleCategoryModeDefaults(containerIn, containerOut, coerce) { | ||
|
||
if(containerIn.type === 'category') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we like early So, if(containerIn.type !== 'category') return; and flatten ⏬ |
||
|
||
var validCategories = ['trace', 'category ascending', 'category descending', 'array']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should var validCategories = layoutAttributes.categorymode.values; to keep things DRY 🌴 |
||
|
||
var properCategoryList = Array.isArray(containerIn.categorylist) && containerIn.categorylist.length > 0; | ||
|
||
if(validCategories.indexOf(containerIn.categorymode) === -1 && properCategoryList) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Coercing while keeping track of whether the user input in
So, no action needed here. This logic works great and is well tested. |
||
|
||
// when unspecified or invalid, use the default, unless categorylist implies 'array' | ||
coerce('categorymode', 'array'); // promote to 'array | ||
|
||
} else if(containerIn.categorymode === 'array' && !properCategoryList) { | ||
|
||
// when mode is 'array' but no list is given, revert to default | ||
|
||
containerIn.categorymode = 'trace'; // revert to default | ||
coerce('categorymode'); | ||
|
||
} else { | ||
|
||
// otherwise use the supplied mode, or the default one if unsupplied or invalid | ||
coerce('categorymode'); | ||
|
||
} | ||
} | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard Is this a reasonable cut at the coercion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice job. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/** | ||
* Copyright 2012-2016, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
|
||
'use strict'; | ||
|
||
var d3 = require('d3'); | ||
|
||
// flattenUnique :: String -> [[String]] -> Object | ||
function flattenUnique(axisLetter, data) { | ||
var traceLines = data.map(function(d) {return d[axisLetter];}); | ||
var categoryMap = {}; // hashmap is O(1); | ||
var i, j, tracePoints, category; | ||
for(i = 0; i < traceLines.length; i++) { | ||
tracePoints = traceLines[i]; | ||
for(j = 0; j < tracePoints.length; j++) { | ||
category = tracePoints[j]; | ||
if(!categoryMap[category]) { | ||
categoryMap[category] = true; | ||
} | ||
} | ||
} | ||
return categoryMap; | ||
} | ||
|
||
// flattenUniqueSort :: String -> Function -> [[String]] -> [String] | ||
function flattenUniqueSort(axisLetter, sortFunction, data) { | ||
return Object.keys(flattenUnique(axisLetter, data)).sort(sortFunction); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard I quite rewrote the ordering logic this evening on initial suspicion that image based cases may fail in presence of a large number of points, because of possible slowness due to the O(N) complexity. I switched to bisection. While it didn't make a difference, better scalability is good anyway. As it changed a good bit, it's useful if you know about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks good 👍 |
||
|
||
/** | ||
* This pure function returns the ordered categories for specified axisLetter, categorymode, categorylist and data. | ||
* | ||
* If categorymode is 'array', the result is a fresh copy of categorylist, or if unspecified, an empty array. | ||
* | ||
* If categorymode is 'category ascending' or 'category descending', the result is an array of ascending or descending | ||
* order of the unique categories encountered in the data for specified axisLetter. | ||
* | ||
* See cartesian/layout_attributes.js for the definition of categorymode and categorylist | ||
* | ||
*/ | ||
|
||
// orderedCategories :: String -> String -> [String] -> [[String]] -> [String] | ||
module.exports = function orderedCategories(axisLetter, categorymode, categorylist, data) { | ||
|
||
switch(categorymode) { | ||
case 'array': return Array.isArray(categorylist) ? categorylist : []; | ||
case 'category ascending': return flattenUniqueSort(axisLetter, d3.ascending, data); | ||
case 'category descending': return flattenUniqueSort(axisLetter, d3.descending, data); | ||
case 'trace': return []; | ||
default: return []; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,8 +182,9 @@ module.exports = function setConvert(ax) { | |
// encounters them, ie all the categories from the | ||
// first data set, then all the ones from the second | ||
// that aren't in the first etc. | ||
// TODO: sorting options - do the sorting | ||
// progressively here as we insert? | ||
// it is assumed that this function is being invoked in the | ||
// already sorted category order; otherwise there would be | ||
// a disconnect between the array and the index returned | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @monfera can you elaborate on this? What else have you tried? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard I tried things like this:
Then I spent a bit of time better understanding concepts, the data flow and the various ways it can be invoked (e.g. plotting multiple lines; overplotting onto an already existing plot etc). Learnt that currently, the categorical X axis ordering is totally driven by the order in which the point tuples arrive and made sample plots. Which was the point at which I gave up on this point for modification and went for the lower hanging fruit of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard maybe an example is helpful: The first vector has X cat. values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard I also considered doing a sorting a lot earlier (more upstream), but it would have issues: 1) the code should possibly reorder only after it's established that it's a categorical axis (it can be user specified or determined heuristically); 2) by that point there are lots of places where So my current thought is that we need to do the axis tick sorting downstream, nearer the point which is responsible for the d3 data binding order for the axis ticks, and we have to accommodate for the possibility that new lines introduce new points that have to be inserted in lines that have already been added previously. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @monfera Thanks for this very detailed overview. You've got me convinced, We need to fill in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filling in We already need to loop all traces to check for box plot irregularities here, so maybe you could fill in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard I'll look into it, thanks! Unrelated: I learnt that date axes take ISO strings There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard I'll look into it more closely today, but on an initial look, the suggested place isn't as trivial (to me) and I'd put it slightly after this point, because
Therefore I'm planning to put the logic after having returned from setAutoType; the earliest point seems to be just before the call to This way, we wouldn't reuse the suggested loop, but the loop over the traces probably doesn't have a measurable performance impact anyway (I'm assuming there usually aren't thousands or 10ks of trace lines). I'll do some more tests to ensure I'm not overlooking something; just wanted to share current thinking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anywhere, in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard Thanks for the quick note, I'll go ahead. |
||
|
||
if(v !== null && v !== undefined && ax._categories.indexOf(v) === -1) { | ||
ax._categories.push(v); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,8 @@ var Color = require('@src/components/color'); | |
var handleTickValueDefaults = require('@src/plots/cartesian/tick_value_defaults'); | ||
var Axes = PlotlyInternal.Axes; | ||
|
||
var createGraph = require('../assets/create_graph_div'); | ||
var destroyGraph = require('../assets/destroy_graph_div'); | ||
var createGraphDiv = require('../assets/create_graph_div'); | ||
var destroyGraphDiv = require('../assets/destroy_graph_div'); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard I appended 'Div' to these, to be in sync with how it's usually called. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nicely done. |
||
|
||
describe('Test axes', function() { | ||
|
@@ -321,15 +321,137 @@ describe('Test axes', function() { | |
}); | ||
}); | ||
|
||
describe('categorymode', function() { | ||
|
||
var gd; | ||
|
||
beforeEach(function() { | ||
gd = createGraphDiv(); | ||
}); | ||
|
||
afterEach(destroyGraphDiv); | ||
|
||
describe('setting, or not setting categorymode if it is not explicitly declared', function() { | ||
|
||
it('should set categorymode to default if categorymode and categorylist are not supplied', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], {xaxis: {type: 'category'}}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('trace'); | ||
}); | ||
|
||
it('should set categorymode to default even if type is not set to category explicitly', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}]); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('trace'); | ||
}); | ||
|
||
it('should NOT set categorymode to default if type is not category', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}]); | ||
expect(gd._fullLayout.yaxis.categorymode).toBe(undefined); | ||
}); | ||
|
||
it('should set categorymode to default if type is overridden to be category', function() { | ||
PlotlyInternal.plot(gd, [{x: [1,2,3,4,5], y: [15,11,12,13,14]}], {yaxis: {type: 'category'}}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe(undefined); | ||
expect(gd._fullLayout.yaxis.categorymode).toBe('trace'); | ||
}); | ||
|
||
}); | ||
|
||
describe('setting categorymode to "array"', function() { | ||
|
||
it('should leave categorymode on "array" if it is supplied', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'array', categorylist: ['b','a','d','e','c']} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('array'); | ||
}); | ||
|
||
it('should switch categorymode on "array" if it is not supplied but categorylist is supplied', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorylist: ['b','a','d','e','c']} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('array'); | ||
}); | ||
|
||
it('should revert categorymode to "trace" if "array" is supplied but there is no list', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'array'} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('trace'); | ||
}); | ||
|
||
}); | ||
|
||
describe('do not set categorymode to "array" if list exists but empty', function() { | ||
|
||
it('should switch categorymode to default if list is not supplied', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'array', categorylist: []} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('trace'); | ||
}); | ||
|
||
it('should not switch categorymode on "array" if categorylist is supplied but empty', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorylist: []} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('trace'); | ||
}); | ||
}); | ||
|
||
describe('do NOT set categorymode to "array" if it has some other proper value', function() { | ||
|
||
it('should use specified categorymode if it is supplied even if categorylist exists', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'trace', categorylist: ['b','a','d','e','c']} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('trace'); | ||
}); | ||
|
||
it('should use specified categorymode if it is supplied even if categorylist exists', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'category ascending', categorylist: ['b','a','d','e','c']} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('category ascending'); | ||
}); | ||
|
||
it('should use specified categorymode if it is supplied even if categorylist exists', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'category descending', categorylist: ['b','a','d','e','c']} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('category descending'); | ||
}); | ||
|
||
}); | ||
|
||
describe('setting categorymode to the default if the value is unexpected', function() { | ||
|
||
it('should switch categorymode to "trace" if mode is supplied but invalid', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'invalid value'} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('trace'); | ||
}); | ||
|
||
it('should switch categorymode to "array" if mode is supplied but invalid and list is supplied', function() { | ||
PlotlyInternal.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { | ||
xaxis: {type: 'category', categorymode: 'invalid value', categorylist: ['b','a','d','e','c']} | ||
}); | ||
expect(gd._fullLayout.xaxis.categorymode).toBe('array'); | ||
}); | ||
|
||
}); | ||
|
||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard May I ask you to take a look at these test cases if they seem to be in line with what you wrote about categorymode coercion earlier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. All the test cases you added (along with d16fe6b) look great! |
||
describe('handleTickDefaults', function() { | ||
var data = [{ x: [1,2,3], y: [3,4,5] }], | ||
gd; | ||
|
||
beforeEach(function() { | ||
gd = createGraph(); | ||
gd = createGraphDiv(); | ||
}); | ||
|
||
afterEach(destroyGraph); | ||
afterEach(destroyGraphDiv); | ||
|
||
it('should set defaults on bad inputs', function() { | ||
var layout = { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice find.