-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Histogram edge cases #2028
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
Merged
Merged
Histogram edge cases #2028
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
a9498bb
fix part 1 of #1978 - keep bin width and trim zeros from 1-bin histog…
alexcjohnson 968f742
fix part 2 of #1978 - autobin size for single-value overlaid histograms
alexcjohnson 9959341
fix histogram2d test for _count field
alexcjohnson bcebc8a
fix sieve modification -> for single-item bar traces
alexcjohnson c057d25
lint
alexcjohnson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,7 +135,7 @@ module.exports = function calc(gd, trace) { | |
break; | ||
} | ||
} | ||
for(i = seriesLen - 1; i > firstNonzero; i--) { | ||
for(i = seriesLen - 1; i >= firstNonzero; i--) { | ||
if(size[i]) { | ||
lastNonzero = i; | ||
break; | ||
|
@@ -149,6 +149,12 @@ module.exports = function calc(gd, trace) { | |
} | ||
} | ||
|
||
if(cd.length === 1) { | ||
// when we collapse to a single bin, calcdata no longer describes bin size | ||
// so we need to explicitly specify it | ||
cd[0].width1 = Axes.tickIncrement(cd[0].p, binSpec.size, false, calendar) - cd[0].p; | ||
} | ||
|
||
arraysToCalcdata(cd, trace); | ||
|
||
return cd; | ||
|
@@ -161,8 +167,9 @@ module.exports = function calc(gd, trace) { | |
* smallest bins of any of the auto values for all histograms grouped/stacked | ||
* together. | ||
*/ | ||
function calcAllAutoBins(gd, trace, pa, mainData) { | ||
function calcAllAutoBins(gd, trace, pa, mainData, _overlayEdgeCase) { | ||
var binAttr = mainData + 'bins'; | ||
var isOverlay = gd._fullLayout.barmode === 'overlay'; | ||
var i, tracei, calendar, firstManual, pos0; | ||
|
||
// all but the first trace in this group has already been marked finished | ||
|
@@ -172,7 +179,9 @@ function calcAllAutoBins(gd, trace, pa, mainData) { | |
} | ||
else { | ||
// must be the first trace in the group - do the autobinning on them all | ||
var traceGroup = getConnectedHistograms(gd, trace); | ||
|
||
// find all grouped traces - in overlay mode each trace is independent | ||
var traceGroup = isOverlay ? [trace] : getConnectedHistograms(gd, trace); | ||
var autoBinnedTraces = []; | ||
|
||
var minSize = Infinity; | ||
|
@@ -196,6 +205,19 @@ function calcAllAutoBins(gd, trace, pa, mainData) { | |
|
||
binSpec = Axes.autoBin(pos0, pa, tracei['nbins' + mainData], false, calendar); | ||
|
||
// Edge case: single-valued histogram overlaying others | ||
// Use them all together to calculate the bin size for the single-valued one | ||
if(isOverlay && binSpec._count === 1 && pa.type !== 'category') { | ||
// trace[binAttr] = binSpec; | ||
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. 🔪 ? 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. thanks - 🔪 in c057d25 |
||
|
||
// Several single-valued histograms! Stop infinite recursion, | ||
// just return an extra flag that tells handleSingleValueOverlays | ||
// to sort out this trace too | ||
if(_overlayEdgeCase) return [binSpec, pos0, true]; | ||
|
||
binSpec = handleSingleValueOverlays(gd, trace, pa, mainData, binAttr); | ||
} | ||
|
||
// adjust for CDF edge cases | ||
if(cumulativeSpec.enabled && (cumulativeSpec.currentbin !== 'include')) { | ||
if(cumulativeSpec.direction === 'decreasing') { | ||
|
@@ -212,9 +234,9 @@ function calcAllAutoBins(gd, trace, pa, mainData) { | |
} | ||
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... | ||
// accommodating of this one, so other bins line up with these. | ||
// But 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), | ||
|
@@ -276,14 +298,90 @@ function calcAllAutoBins(gd, trace, pa, mainData) { | |
} | ||
|
||
/* | ||
* Return an array of traces that are all stacked or grouped together | ||
* Only considers histograms. In principle we could include them in a | ||
* Adjust single-value histograms in overlay mode to make as good a | ||
* guess as we can at autobin values the user would like. | ||
* | ||
* Returns the binSpec for the trace that sparked all this | ||
*/ | ||
function handleSingleValueOverlays(gd, trace, pa, mainData, binAttr) { | ||
var overlaidTraceGroup = getConnectedHistograms(gd, trace); | ||
var pastThisTrace = false; | ||
var minSize = Infinity; | ||
var singleValuedTraces = [trace]; | ||
var i, tracei; | ||
|
||
// first collect all the: | ||
// - min bin size from all multi-valued traces | ||
// - single-valued traces | ||
for(i = 0; i < overlaidTraceGroup.length; i++) { | ||
tracei = overlaidTraceGroup[i]; | ||
if(tracei === trace) pastThisTrace = true; | ||
else if(!pastThisTrace) { | ||
// This trace has already had its autobins calculated | ||
// (so must not have been single-valued). | ||
minSize = Math.min(minSize, tracei[binAttr].size); | ||
} | ||
else { | ||
var resulti = calcAllAutoBins(gd, tracei, pa, mainData, true); | ||
var binSpeci = resulti[0]; | ||
var isSingleValued = resulti[2]; | ||
|
||
// so we can use this result when we get to tracei in the normal | ||
// course of events, mark it as done and put _pos0 back | ||
tracei._autoBinFinished = 1; | ||
tracei._pos0 = resulti[1]; | ||
|
||
if(isSingleValued) { | ||
singleValuedTraces.push(tracei); | ||
} | ||
else { | ||
minSize = Math.min(minSize, binSpeci.size); | ||
} | ||
} | ||
} | ||
|
||
// find the real data values for each single-valued trace | ||
// hunt through pos0 for the first valid value | ||
var dataVals = new Array(singleValuedTraces.length); | ||
for(i = 0; i < singleValuedTraces.length; i++) { | ||
var pos0 = singleValuedTraces[i]._pos0; | ||
for(var j = 0; j < pos0.length; j++) { | ||
if(pos0[j] !== undefined) { | ||
dataVals[i] = pos0[j]; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
// are ALL traces are single-valued? use the min difference between | ||
// all of their values (which defaults to 1 if there's still only one) | ||
if(!isFinite(minSize)) { | ||
minSize = Lib.distinctVals(dataVals).minDiff; | ||
} | ||
|
||
// now apply the min size we found to all single-valued traces | ||
for(i = 0; i < singleValuedTraces.length; i++) { | ||
tracei = singleValuedTraces[i]; | ||
var calendar = tracei[mainData + 'calendar']; | ||
|
||
tracei._input[binAttr] = tracei[binAttr] = { | ||
start: pa.c2r(dataVals[i] - minSize / 2, 0, calendar), | ||
end: pa.c2r(dataVals[i] + minSize / 2, 0, calendar), | ||
size: minSize | ||
}; | ||
} | ||
|
||
return trace[binAttr]; | ||
} | ||
|
||
/* | ||
* Return an array of histograms that share axes and orientation. | ||
* | ||
* Only considers histograms. In principle we could include bars in a | ||
* similar way to how we do manually binned histograms, though this | ||
* would have tons of edge cases and value judgments to make. | ||
*/ | ||
function getConnectedHistograms(gd, trace) { | ||
if(gd._fullLayout.barmode === 'overlay') return [trace]; | ||
|
||
var xid = trace.xaxis; | ||
var yid = trace.yaxis; | ||
var orientation = trace.orientation; | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@n-riesco you wrote some fantastic tests of bar chart edge cases, really saved us here!