-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add Aggregation user input _index #2031
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
Changes from all commits
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 |
---|---|---|
|
@@ -190,6 +190,26 @@ describe('aggregate', function() { | |
expect(traceOut.marker.size).toEqual([10, 20]); | ||
}); | ||
|
||
// Regression test - throws before fix: | ||
// https://github.com/plotly/plotly.js/issues/2024 | ||
it('can handle case where aggregation array is missing', function() { | ||
Plotly.newPlot(gd, [{ | ||
x: [1, 2, 3, 4, 5], | ||
y: [2, 4, 6, 8, 10], | ||
marker: {size: [10, 10, 20, 20, 10]}, | ||
transforms: [{ | ||
type: 'aggregate', | ||
groups: 'marker.size' | ||
}] | ||
}]); | ||
|
||
var traceOut = gd._fullData[0]; | ||
|
||
expect(traceOut.x).toEqual([1, 3]); | ||
expect(traceOut.y).toEqual([2, 6]); | ||
expect(traceOut.marker.size).toEqual([10, 20]); | ||
}); | ||
|
||
it('handles median, mode, rms, & stddev for numeric data', function() { | ||
// again, nothing is going to barf with non-numeric data, but sometimes it | ||
// won't make much sense. | ||
|
@@ -225,4 +245,40 @@ describe('aggregate', function() { | |
expect(traceOut.marker.line.width).toBeCloseToArray([0.5, 0], 5); | ||
expect(traceOut.marker.color).toBeCloseToArray([Math.sqrt(1 / 3), 0], 5); | ||
}); | ||
|
||
it('links fullData aggregations to userData via _index', function() { | ||
Plotly.newPlot(gd, [{ | ||
x: [1, 2, 3, 4, 5], | ||
y: [2, 4, 6, 8, 10], | ||
marker: { | ||
size: [10, 10, 20, 20, 10], | ||
color: ['red', 'green', 'blue', 'yellow', 'white'] | ||
}, | ||
transforms: [{ | ||
type: 'aggregate', | ||
groups: 'marker.size', | ||
aggregations: [ | ||
{target: 'x', func: 'sum'}, | ||
{target: 'x', func: 'avg'}, | ||
{target: 'y', func: 'avg'}, | ||
] | ||
}] | ||
}]); | ||
|
||
var traceOut = gd._fullData[0]; | ||
var fullAggregation = traceOut.transforms[0]; | ||
var fullAggregations = fullAggregation.aggregations; | ||
var enabledAggregations = fullAggregations.filter(function(agg) { | ||
return agg.enabled; | ||
}); | ||
|
||
expect(enabledAggregations[0].target).toEqual('x'); | ||
expect(enabledAggregations[0]._index).toEqual(0); | ||
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. Is there any way to test this that doesn't rely upon an underscored property? Like could 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. The only reason for indicies is for Workspace. supplyTransformDefaults doesn't care about them. 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. I need a way in Workspace to take a fullData transform Aggregration and write back to the correct userData index. I need some way to correlate userData indicies with the fullData equivalents. 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. maybe it shouldn't be underscored? Something like 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. So the workspace accesses indices directly? (That's okay, just making sure. plotly.js's 'private' properties are fairly publicly accessed, so it's nothing new. Just means they need to be modified with great care.) 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. yep, and the workspace is the only thing that needs them. Plotly.js doesn't care as it rebuilds new arrays each time. |
||
|
||
expect(enabledAggregations[1].target).toEqual('y'); | ||
expect(enabledAggregations[1]._index).toEqual(2); | ||
|
||
expect(enabledAggregations[2].target).toEqual('marker.color'); | ||
expect(enabledAggregations[2]._index).toEqual(-1); | ||
}); | ||
}); |
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.
Questions:
supplyTraceDefaults
runs on traces and then again on expanded traces?_module
is added? I think that sometimes refers to the trace module and sometimes to the transform module (and every now and then to the base plot module, though usually it's calledbaseModule
or something, I think)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.
Answer to second: the transform module.
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.
Yep that is what it is referring to