From 1d6874245311228184bb1cfcd378161a76e7fa89 Mon Sep 17 00:00:00 2001 From: William Finzer Date: Mon, 16 Dec 2024 08:05:57 -0800 Subject: [PATCH] Merge pull request #1685 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [#188590950] Bug fix: Graph: crash on axis switch from categorical pa… * * This change fixes a cypress test that failed. The problem was that … --- .../count/count-adornment-component.tsx | 4 +- .../graph/components/binneddotplotdots.tsx | 10 +- .../components/graph/components/histogram.tsx | 2 + v3/src/components/graph/hooks/use-dot-plot.ts | 4 +- .../graph/models/graph-content-model.ts | 131 +++++++++--------- 5 files changed, 81 insertions(+), 70 deletions(-) diff --git a/v3/src/components/graph/adornments/count/count-adornment-component.tsx b/v3/src/components/graph/adornments/count/count-adornment-component.tsx index 541be23f5b..d60cd5954b 100644 --- a/v3/src/components/graph/adornments/count/count-adornment-component.tsx +++ b/v3/src/components/graph/adornments/count/count-adornment-component.tsx @@ -46,13 +46,13 @@ export const CountAdornment = observer(function CountAdornment(props: IAdornment // movable values present at the same time. if (graphModel.pointDisplayType === "bins") { const { binWidth, minBinEdge, maxBinEdge, totalNumberOfBins } = graphModel.binDetails() - return [ + return binWidth !== undefined ? [ // Build and spread an array of numeric values corresponding to the bin boundaries. Using totalNumberOfBins // for length, start at minBinEdge and increment by binWidth using each bin's index. Afterward, add // maxBinEdge to complete the region boundaries array. ...Array.from({ length: totalNumberOfBins }, (_, i) => minBinEdge + i * binWidth), maxBinEdge - ] + ] : [] } return adornmentsStore?.subPlotRegionBoundaries(instanceKey, scale) ?? [] }, [adornmentsStore, graphModel, instanceKey, scale]) diff --git a/v3/src/components/graph/components/binneddotplotdots.tsx b/v3/src/components/graph/components/binneddotplotdots.tsx index e50e85cc1d..c37714f7a6 100644 --- a/v3/src/components/graph/components/binneddotplotdots.tsx +++ b/v3/src/components/graph/components/binneddotplotdots.tsx @@ -47,6 +47,8 @@ export const BinnedDotPlotDots = observer(function BinnedDotPlotDots(props: Plot const { binWidth, minBinEdge, totalNumberOfBins } = graphModel.binDetails() const binBoundariesArea = select(binBoundariesRef.current) + if (binWidth === undefined) return + binBoundariesArea.selectAll("path").remove() for (let i = 1; i < totalNumberOfBins; i++) { const primaryBoundaryOrigin = primaryAxisScale(minBinEdge + i * binWidth) @@ -82,9 +84,11 @@ export const BinnedDotPlotDots = observer(function BinnedDotPlotDots(props: Plot primaryAxisScaleCopy.current = primaryAxisScale.copy() graphModel.setDragBinIndex(binIndex) const { binWidth, minBinEdge } = graphModel.binDetails() - const newBinAlignment = minBinEdge + binIndex * binWidth - lowerBoundaryRef.current = primaryAxisScale(newBinAlignment) - graphModel.setDynamicBinAlignment(newBinAlignment) + if (binWidth !== undefined) { + const newBinAlignment = minBinEdge + binIndex * binWidth + lowerBoundaryRef.current = primaryAxisScale(newBinAlignment) + graphModel.setDynamicBinAlignment(newBinAlignment) + } }, [dataConfig, graphModel, primaryAxisScale]) const handleDragBinBoundary = useCallback((event: MouseEvent) => { diff --git a/v3/src/components/graph/components/histogram.tsx b/v3/src/components/graph/components/histogram.tsx index 27db8b1f6b..09f03d2cc7 100644 --- a/v3/src/components/graph/components/histogram.tsx +++ b/v3/src/components/graph/components/histogram.tsx @@ -49,6 +49,8 @@ export const Histogram = observer(function Histogram({ abovePointsGroupRef, pixi numExtraPrimaryBands, pointDiameter, primaryAttrID, primaryAxisScale, primaryPlace, secondaryAttrID, secondaryBandwidth, totalNumberOfBins } + if (binWidth === undefined) return + const { bins } = computeBinPlacements(binPlacementProps) const primaryScaleDiff = primaryAxisScale(binWidth) - primaryAxisScale(0) const getWidth = () => primaryIsBottom ? primaryScaleDiff / numExtraPrimaryBands : secondaryNumericUnitLength diff --git a/v3/src/components/graph/hooks/use-dot-plot.ts b/v3/src/components/graph/hooks/use-dot-plot.ts index 632be6d0aa..4cab01481e 100644 --- a/v3/src/components/graph/hooks/use-dot-plot.ts +++ b/v3/src/components/graph/hooks/use-dot-plot.ts @@ -86,7 +86,7 @@ export const useDotPlot = (pixiPoints?: PixiPoints) => { const { primaryCoord, extraPrimaryCoord } = computePrimaryCoord(computePrimaryCoordProps) let primaryScreenCoord = primaryCoord + extraPrimaryCoord - if (graphModel.pointDisplayType !== "histogram") { + if (binWidth !== undefined && graphModel.pointDisplayType !== "histogram") { const caseValue = dataDisplayGetNumericValue(dataset, anID, primaryAttrID) ?? -1 const binForCase = determineBinForCase(caseValue, binWidth, minBinEdge) primaryScreenCoord = adjustCoordForStacks({ @@ -111,7 +111,7 @@ export const useDotPlot = (pixiPoints?: PixiPoints) => { } let secondaryScreenCoord = computeSecondaryCoord(secondaryCoordProps) - if (graphModel.pointDisplayType !== "histogram") { + if (binWidth !== undefined && graphModel.pointDisplayType !== "histogram") { const onePixelOffset = primaryIsBottom ? -1 : 1 const casePrimaryValue = dataDisplayGetNumericValue(dataset, anID, primaryAttrID) ?? -1 const binForCase = determineBinForCase(casePrimaryValue, binWidth, minBinEdge) diff --git a/v3/src/components/graph/models/graph-content-model.ts b/v3/src/components/graph/models/graph-content-model.ts index ef8f700ee5..7cc49361ec 100644 --- a/v3/src/components/graph/models/graph-content-model.ts +++ b/v3/src/components/graph/models/graph-content-model.ts @@ -120,6 +120,13 @@ export const GraphContentModel = DataDisplayContentModel setPointOverlap(overlap: number) { self.pointOverlap = overlap }, + setBinWidth(width: number | undefined) { + self._binWidth = isFiniteNumber(width) ? width : undefined + self.dynamicBinWidth = undefined + }, + setDynamicBinWidth(width: number) { + self.dynamicBinWidth = width + }, })) .views(self => ({ get graphPointLayerModel(): IGraphPointLayerModel { @@ -177,8 +184,8 @@ export const GraphContentModel = DataDisplayContentModel clampPosMinAtZero: self.pointDisplayType === "bars" || self.pointsFusedIntoBars } }, - binWidthFromData(minValue: number, maxValue: number): number { - if (minValue === Infinity || maxValue === -Infinity) return 1 + binWidthFromData(minValue: number, maxValue: number) { + if (minValue === Infinity || maxValue === -Infinity || minValue === maxValue) return undefined const kDefaultNumberOfBins = 4 const binRange = maxValue !== minValue @@ -217,18 +224,16 @@ export const GraphContentModel = DataDisplayContentModel const maxValue = caseDataArray.reduce((max, aCaseData) => { return Math.max(max, dataDisplayGetNumericValue(dataset, aCaseData.caseID, primaryAttributeID) ?? max) }, -Infinity) - - if (minValue === Infinity || maxValue === -Infinity) { - return { binAlignment: 0, binWidth: 1, minBinEdge: 0, maxBinEdge: 0, minValue: 0, maxValue: 0, - totalNumberOfBins: 0 } + const binWidth = (initialize || !self.binWidth) + ? self.binWidthFromData(minValue, maxValue) : self.binWidth + if (minValue === Infinity || maxValue === -Infinity || binWidth === undefined) { + return { binAlignment: 0, binWidth: undefined, minBinEdge: 0, maxBinEdge: 0, minValue: 0, maxValue: 0, + totalNumberOfBins: 0 } } - const binWidth = initialize || !self.binWidth - ? self.binWidthFromData(minValue, maxValue) - : self.binWidth const binAlignment = initialize || !self.binAlignment - ? Math.floor(minValue / binWidth) * binWidth - : self.binAlignment + ? Math.floor(minValue / binWidth) * binWidth + : self.binAlignment const minBinEdge = binAlignment - Math.ceil((binAlignment - minValue) / binWidth) * binWidth // Calculate the total number of bins needed to cover the range from the minimum data value // to the maximum data value, adding a small constant to ensure the max value is contained. @@ -236,7 +241,7 @@ export const GraphContentModel = DataDisplayContentModel const maxBinEdge = minBinEdge + (totalNumberOfBins * binWidth) return { binAlignment, binWidth, minBinEdge, maxBinEdge, minValue, maxValue, totalNumberOfBins } - } + }, })) .actions(self => ({ afterCreate() { @@ -294,34 +299,28 @@ export const GraphContentModel = DataDisplayContentModel setDynamicBinAlignment(alignment: number) { self.dynamicBinAlignment = alignment }, - setBinWidth(width: number) { - self._binWidth = isFiniteNumber(width) ? width : undefined - self.dynamicBinWidth = undefined - }, - setDynamicBinWidth(width: number) { - self.dynamicBinWidth = width - }, binnedAxisTicks(formatter?: (value: number) => string): { tickValues: number[], tickLabels: string[] } { const tickValues: number[] = [] const tickLabels: string[] = [] const { binWidth, totalNumberOfBins, minBinEdge } = self.binDetails() + if (binWidth !== undefined) { + let currentStart = minBinEdge + let binCount = 0 - let currentStart = minBinEdge - let binCount = 0 - - while (binCount < totalNumberOfBins) { - const currentEnd = currentStart + binWidth - if (formatter) { - const formattedCurrentStart = formatter(currentStart) - const formattedCurrentEnd = formatter(currentEnd) - tickValues.push(currentStart + (binWidth / 2)) - tickLabels.push(`[${formattedCurrentStart}, ${formattedCurrentEnd})`) - } else { - tickValues.push(currentStart + binWidth) - tickLabels.push(`${currentEnd}`) + while (binCount < totalNumberOfBins) { + const currentEnd = currentStart + binWidth + if (formatter) { + const formattedCurrentStart = formatter(currentStart) + const formattedCurrentEnd = formatter(currentEnd) + tickValues.push(currentStart + (binWidth / 2)) + tickLabels.push(`[${formattedCurrentStart}, ${formattedCurrentEnd})`) + } else { + tickValues.push(currentStart + binWidth) + tickLabels.push(`${currentEnd}`) + } + currentStart += binWidth + binCount++ } - currentStart += binWidth - binCount++ } return { tickValues, tickLabels } }, @@ -343,17 +342,19 @@ export const GraphContentModel = DataDisplayContentModel const tickLabels: string[] = [] const { binWidth, totalNumberOfBins, minBinEdge } = self.binDetails() - let currentStart = minBinEdge - let binCount = 0 + if (binWidth !== undefined) { + let currentStart = minBinEdge + let binCount = 0 - while (binCount < totalNumberOfBins) { - const currentEnd = currentStart + binWidth - const formattedCurrentStart = formatter(currentStart) - const formattedCurrentEnd = formatter(currentEnd) - tickValues.push(currentStart + (binWidth / 2)) - tickLabels.push(`[${formattedCurrentStart}, ${formattedCurrentEnd})`) - currentStart += binWidth - binCount++ + while (binCount < totalNumberOfBins) { + const currentEnd = currentStart + binWidth + const formattedCurrentStart = formatter(currentStart) + const formattedCurrentEnd = formatter(currentEnd) + tickValues.push(currentStart + (binWidth / 2)) + tickLabels.push(`[${formattedCurrentStart}, ${formattedCurrentEnd})`) + currentStart += binWidth + binCount++ + } } return { tickValues, tickLabels } }, @@ -374,12 +375,14 @@ export const GraphContentModel = DataDisplayContentModel if (self.pointDisplayType === "histogram") { const { binWidth, minBinEdge } = self.binDetails() - const binIndex = Math.floor((Number(value) - minBinEdge) / binWidth) - matchingCases = allCases?.filter(aCase => { - const caseValue = dataDisplayGetNumericValue(dataset, aCase.__id__, attrID) ?? 0 - const bin = Math.floor((caseValue - minBinEdge) / binWidth) - return bin === binIndex - }) as ICase[] ?? [] + if (binWidth !== undefined) { + const binIndex = Math.floor((Number(value) - minBinEdge) / binWidth) + matchingCases = allCases?.filter(aCase => { + const caseValue = dataDisplayGetNumericValue(dataset, aCase.__id__, attrID) ?? 0 + const bin = Math.floor((caseValue - minBinEdge) / binWidth) + return bin === binIndex + }) as ICase[] ?? [] + } } else if (attrID && value) { matchingCases = allCases?.filter(aCase => dataset?.getStrValue(aCase.__id__, attrID) === value) as ICase[] ?? [] } @@ -425,18 +428,20 @@ export const GraphContentModel = DataDisplayContentModel return true }) const { binWidth, minBinEdge } = self.binDetails() - const binIndex = Math.floor((Number(casePrimaryValue) - minBinEdge) / binWidth) - const firstCount = allMatchingCases.length - const secondCount = casesInSubPlot.length - const percent = float(100 * firstCount / secondCount) - const minBinValue = minBinEdge + binIndex * binWidth - const maxBinValue = minBinEdge + (binIndex + 1) * binWidth - // " of (

%) are ≥ L and < U" - const attrArray = [ firstCount, secondCount, percent, minBinValue, maxBinValue ] - const translationKey = firstCount === 1 - ? "DG.HistogramView.barTipNoLegendSingular" - : "DG.HistogramView.barTipNoLegendPlural" - tipText = t(translationKey, {vars: attrArray}) + if (binWidth !== undefined) { + const binIndex = Math.floor((Number(casePrimaryValue) - minBinEdge) / binWidth) + const firstCount = allMatchingCases.length + const secondCount = casesInSubPlot.length + const percent = float(100 * firstCount / secondCount) + const minBinValue = minBinEdge + binIndex * binWidth + const maxBinValue = minBinEdge + (binIndex + 1) * binWidth + // " of (

%) are ≥ L and < U" + const attrArray = [firstCount, secondCount, percent, minBinValue, maxBinValue] + const translationKey = firstCount === 1 + ? "DG.HistogramView.barTipNoLegendSingular" + : "DG.HistogramView.barTipNoLegendPlural" + tipText = t(translationKey, {vars: attrArray}) + } } else { const topSplitMatches = self.matchingCasesForAttr(topSplitAttrID, caseTopSplitValue) const rightSplitMatches = self.matchingCasesForAttr(rightSplitAttrID, caseRightSplitValue) @@ -645,7 +650,7 @@ export const GraphContentModel = DataDisplayContentModel const secondaryPlace = secondaryRole === "y" ? "left" : "bottom" const extraPrimAttrRole = primaryRole === "x" ? "topSplit" : "rightSplit" const extraSecAttrRole = primaryRole === "x" ? "rightSplit" : "topSplit" - const maxCellCaseCount = self.pointDisplayType === "histogram" + const maxCellCaseCount = (self.pointDisplayType === "histogram" && binWidth !== undefined) ? maxCellLength(extraPrimAttrRole, extraSecAttrRole, binWidth, minValue, totalNumberOfBins) : maxOverAllCells(extraPrimAttrRole, extraSecAttrRole) const countAxis = NumericAxisModel.create({