8000 Match hist bins by alexcjohnson · Pull Request #1944 · plotly/plotly.js · GitHub
[go: up one dir, main page]
Skip to content

Match hist bins #1944

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

Merged
merged 8 commits into from
Aug 15, 2017
Merged

Match hist bins #1944

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e5a535c
histogram autobins match each other when possible
alexcjohnson Aug 8, 2017
11821c2
extra test for histograms on other axes
alexcjohnson Aug 10, 2017
ec12eab
clear - and test - TODOs we're not going to address now
alexcjohnson Aug 14, 2017
3f173a4
test new histogram logic in images
alexcjohnson Aug 14, 2017
89093ad
handle _pos0 totally inside calcAllAutoBins
alexcjohnson Aug 14, 2017
2071acb
camelCase histogram.calc
alexcjohnson Aug 14, 2017
e81fcb6
cut out duplicate setPositions calls (and loops)
alexcjohnson Aug 14, 2017
241d9a8
pushUnique setPositions
alexcjohnson Aug 15, 2017
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
Prev Previous commit
Next Next commit
handle _pos0 totally inside calcAllAutoBins
  • Loading branch information
@alexcjohnson
alexcjohnson committed Aug 14, 2017
commit 89093ad688568d229baa967b440a7a8cd92970be
192 changes: 95 additions & 97 deletions src/traces/histogram/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,9 @@ module.exports = function calc(gd, trace) {

cleanBins(trace, pa, maindata);

var binspec = calcAllAutoBins(gd, trace, pa, maindata);

// the raw data was prepared in calcAllAutoBins (during the first trace in
// this group) and stashed. Pull it out and drop the stash
var pos0 = trace._pos0;
delete trace._pos0;
var binsAndPos = calcAllAutoBins(gd, trace, pa, maindata);
var binspec = binsAndPos[0];
var pos0 = binsAndPos[1];

var nonuniformBins = typeof binspec.size === 'string';
var bins = nonuniformBins ? [] : binspec;
Expand Down Expand Up @@ -166,115 +163,116 @@ module.exports = function calc(gd, trace) {
*/
function calcAllAutoBins(gd, trace, pa, maindata) {
var binAttr = maindata + 'bins';
var i, tracei, calendar, firstManual, pos0;

// all but the first trace in this group has already been marked finished
// clear this flag, so next time we run calc we will run autobin again
if(trace._autoBinFinished) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should move auto-bins computations to histograms/set_positions (which loops over all traces of a given trace type) to avoid having to use _ flags like this one?

Copy link
Contributor
@etpinard etpinard Aug 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

... maybe histogram/calc.js should only sanitize pos and size values and call arraysToCalcdata, and leave the rest to set_positions.js?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe histogram/calc.js should only sanitize pos and size values and call arraysToCalcdata, and leave the rest to set_positions.js?

Pro: that would avoid coupling between traces in a part of the code (calc) that's supposed to be operating on one trace at a time... and set_positions is only called after calc (as opposed to every replot), which is when it's needed.

Con: we'd need to be saving these sanitized pos0 during calc, rather than making an actual calcdata[i] array - or perhaps stashing it in calcdata[i][0] as we used to do with trace-wide stuff (and still do here and there) but then setPositions would need to fill in the rest of calcdata[i] once it determined the bin spec.

So I feel like in the end it would be more confusing that way. Thoughts?

Incidentally, it looks like we're probably calling setPositions twice unnecessarily if we have both histogram and bar traces on the same plot. I'll investigate...

Copy link
Contributor

Choose a reason for hiding this comment

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

So I feel like in the end it would be more confusing that way. Thoughts?

Good point. I don't have a strong opinion. This whole calc vs set_positions debate was just something that came to mind, not anything blocking that's for sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK cool - I'm going to leave it then, but this discussion will be useful to keep in mind for the future, I suspect at some point we will want to reimagine the whole pipeline to be a bit more flexible - particularly in regards to minimizing redraw work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Incidentally, it looks like we're probably calling setPositions twice unnecessarily if we have both histogram and bar traces on the same plot. I'll investigate...

Yes we were. Fixed in e81fcb6 - I couldn't think of an easy way to test that we're not doing extra work, but we do have a test that this didn't break anything, in bar_and_histogram.json. I also had it avoid even doing the setPositions loop when there are no setPositions functions to call.

delete trace._autoBinFinished;

return trace[binAttr];
}
else {
// must be the first trace in the group - do the autobinning on them all
var traceGroup = getConnectedHistograms(gd, trace);
var autoBinnedTraces = [];

var minSize = Infinity;
var minStart = Infinity;
var maxEnd = -Infinity;

var autoBinAttr = 'autobin' + maindata;

for(i = 0; i < traceGroup.length; i++) {
tracei = traceGroup[i];

// stash pos0 on the trace so we don't need to duplicate this
// in the main body of calc
pos0 = tracei._pos0 = pa.makeCalcdata(tracei, maindata);
var binspec = tracei[binAttr];

if((tracei[autoBinAttr]) || !binspec ||
binspec.start === null || binspec.end === null) {
calendar = tracei[maindata + 'calendar'];
var cumulativeSpec = tracei.cumulative;

binspec = Axes.autoBin(pos0, pa, tracei['nbins' + maindata], false, calendar);

// adjust for CDF edge cases
if(cumulativeSpec.enabled && (cumulativeSpec.currentbin !== 'include')) {
if(cumulativeSpec.direction === 'decreasing') {
minStart = Math.min(minStart, pa.r2c(binspec.start, 0, calendar) - binspec.size);
}
else {
maxEnd = Math.max(maxEnd, pa.r2c(binspec.end, 0, calendar) + binspec.size);
}
}

// must be the first trace in the group - do the autobinning on them all
var traceGroup = getConnectedHistograms(gd, trace);
var autoBinnedTraces = [];

var minSize = Infinity;
var minStart = Infinity;
var maxEnd = -Infinity;

var autoBinAttr = 'autobin' + maindata;
var i, tracei, calendar, firstManual;

// note that it's possible to get here with an explicit autobin: false
// if the bins were not specified. mark this trace for followup
autoBinnedTraces.push(tracei);
}
else if(!firstManual) {
// Remember the first manually set binspec. We'll try to be extra
// accommodating of this one, so other bins line up with these
// if there's more than one manual bin set and they're mutually inconsistent,
// then there's not much we can do...
firstManual = {
size: binspec.size,
start: pa.r2c(binspec.start, 0, calendar),
end: pa.r2c(binspec.end, 0, calendar)
};
}

for(i = 0; i < traceGroup.length; i++) {
tracei = traceGroup[i];
// Even non-autobinned traces get included here, so we get the greatest extent
// and minimum bin size of them all.
// But manually binned traces won't be adjusted, even if the auto values
// are inconsistent with the manual ones (or the manual ones are inconsistent
// with each other).
minSize = getMinSize(minSize, binspec.size);
minStart = Math.min(minStart, pa.r2c(binspec.start, 0, calendar));
maxEnd = Math.max(maxEnd, pa.r2c(binspec.end, 0, calendar));

// add the flag that lets us abort autobin on later traces
if(i) trace._autoBinFinished = 1;
}

// stash pos0 on the trace so we don't need to duplicate this
// in the main body of calc
var pos0 = tracei._pos0 = pa.makeCalcdata(tracei, maindata);
var binspec = tracei[binAttr];
// do what we can to match the auto bins to the first manual bins
// but only if sizes are all numeric
if(firstManual && isNumeric(firstManual.size) && isNumeric(minSize)) {
// first need to ensure the bin size is the same as or an integer fraction
// of the first manual bin
// allow the bin size to increase just under the autobin step size to match,
// (which is a factor of 2 or 2.5) otherwise shrink it
if(minSize > firstManual.size / 1.9) minSize = firstManual.size;
else minSize = firstManual.size / Math.ceil(firstManual.size / minSize);

// now decrease minStart if needed to make the bin centers line up
var adjustedFirstStart = firstManual.start + (firstManual.size - minSize) / 2;
minStart = adjustedFirstStart - minSize * Math.ceil((adjustedFirstStart - minStart) / minSize);
}

if((tracei[autoBinAttr]) || !binspec ||
binspec.start === null || binspec.end === null) {
// now go back to the autobinned traces and update their bin specs with the final values
for(i = 0; i < autoBinnedTraces.length; i++) {
tracei = autoBinnedTraces[i];
calendar = tracei[maindata + 'calendar'];
var cumulativeSpec = tracei.cumulative;

binspec = Axes.autoBin(pos0, pa, tracei['nbins' + maindata], false, calendar);

// adjust for CDF edge cases
if(cumulativeSpec.enabled && (cumulativeSpec.currentbin !== 'include')) {
if(cumulativeSpec.direction === 'decreasing') {
minStart = Math.min(minStart, pa.r2c(binspec.start, 0, calendar) - binspec.size);
}
else {
maxEnd = Math.max(maxEnd, pa.r2c(binspec.end, 0, calendar) + binspec.size);
}
}
tracei._input[binAttr] = tracei[binAttr] = {
start: pa.c2r(minStart, 0, calendar),
end: pa.c2r(maxEnd, 0, calendar),
size: minSize
};

// note that it's possible to get here with an explicit autobin: false
// if the bins were not specified. mark this trace for followup
autoBinnedTraces.push(tracei);
// if the bins were not specified.
// in that case this will remain in the trace, so that future updates
// which would change the autobinning will not do so.
tracei._input[autoBinAttr] = tracei[autoBinAttr];
}
else if(!firstManual) {
// Remember the first manually set binspec. We'll try to be extra
// accommodating of this one, so other bins line up with these
// if there's more than one manual bin set and they're mutually inconsistent,
// then there's not much we can do...
firstManual = {
size: binspec.size,
start: pa.r2c(binspec.start, 0, calendar),
end: pa.r2c(binspec.end, 0, calendar)
};
}

// Even non-autobinned traces get included here, so we get the greatest extent
// and minimum bin size of them all.
// But manually binned traces won't be adjusted, even if the auto values
// are inconsistent with the manual ones (or the manual ones are inconsistent
// with each other).
minSize = getMinSize(minSize, binspec.size);
minStart = Math.min(minStart, pa.r2c(binspec.start, 0, calendar));
maxEnd = Math.max(maxEnd, pa.r2c(binspec.end, 0, calendar));

// add the flag that lets us abort autobin on later traces
if(i) trace._autoBinFinished = 1;
}

// do what we can to match the auto bins to the first manual bins
// but only if sizes are all numeric
if(firstManual && isNumeric(firstManual.size) && isNumeric(minSize)) {
// first need to ensure the bin size is the same as or an integer fraction
// of the first manual bin
// allow the bin size to increase just under the autobin step size to match,
// (which is a factor of 2 or 2.5) otherwise shrink it
if(minSize > firstManual.size / 1.9) minSize = firstManual.size;
else minSize = firstManual.size / Math.ceil(firstManual.size / minSize);

// now decrease minStart if needed to make the bin centers line up
var adjustedFirstStart = firstManual.start + (firstManual.size - minSize) / 2;
minStart = adjustedFirstStart - minSize * Math.ceil((adjustedFirstStart - minStart) / minSize);
}

// now go back to the autobinned traces and update their bin specs with the final values
for(i = 0; i < autoBinnedTraces.length; i++) {
tracei = autoBinnedTraces[i];
calendar = tracei[maindata + 'calendar'];

tracei._input[binAttr] = tracei[binAttr] = {
start: pa.c2r(minStart, 0, calendar),
end: pa.c2r(maxEnd, 0, calendar),
size: minSize
};

// note that it's possible to get here with an explicit autobin: false
// if the bins were not specified.
// in that case this will remain in the trace, so that future updates
// which would change the autobinning will not do so.
tracei._input[autoBinAttr] = tracei[autoBinAttr];
}
pos0 = trace._pos0;
delete trace._pos0;

return trace[binAttr];
return [trace[binAttr], pos0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. Thanks!

}

/*
Expand Down
0