diff --git a/CHANGELOG.md b/CHANGELOG.md index 46de676d5..1b4dd21e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fixed the behavior of `MATCH`, `VLOOKUP`, `HLOOKUP`, and `XLOOKUP` functions when the search range contained empty cells. [#1697](https://github.com/handsontable/hyperformula/pull/1697) - Fixed the `VLOOKUP`, `HLOOKUP`, and `XLOOKUP` functions to return `0` instead of an empty value when the matched cell in the result range is empty. [#1697](https://github.com/handsontable/hyperformula/pull/1697) - Fixed the page freezing when entering a long string of digits containing a non-digit character near the end (e.g. `012...789a` or `012...789 123`) into a cell. [#1520](https://github.com/handsontable/hyperformula/issues/1520) +- Fixed the MAXPOOL and MEDIANPOOL functions throwing an uncaught `TypeError` instead of returning the `#VALUE!` error when the range dimensions are not a whole multiple of the window size and the stride. ## [3.3.0] - 2026-05-20 diff --git a/docs/guide/built-in-functions.md b/docs/guide/built-in-functions.md index 51846e097..f902b86d4 100644 --- a/docs/guide/built-in-functions.md +++ b/docs/guide/built-in-functions.md @@ -348,6 +348,14 @@ Total number of functions: **{{ $page.functionsCount }}** | MAXPOOL | Calculates a smaller range which is a maximum of a Window_size, in a given Range, for every Stride element. | MAXPOOL(Range, Window_size, Stride) | | TRANSPOSE | Transposes the rows and columns of an array. | TRANSPOSE(Array) | +::: tip +`MAXPOOL` and `MEDIANPOOL` require the pooling window to tile `Range` exactly: `Window_size` must be a positive +integer that is not greater than either dimension of `Range`, `Stride` must be a positive integer (defaulting to +`Window_size`), and both dimensions of `Range`, reduced by `Window_size`, must be whole multiples of `Stride`. +Otherwise, the functions return the `#VALUE!` error. For example, `MAXPOOL(A1:C3, 2)` returns `#VALUE!`, because a +window of 2 cells cannot tile a range of 3 cells, while `MAXPOOL(A1:C3, 2, 1)` and `MAXPOOL(A1:D4, 2)` are valid. +::: + ### Operator | Function ID | Description | Syntax | diff --git a/src/error-message.ts b/src/error-message.ts index 5e3afdbea..4b80ee06f 100644 --- a/src/error-message.ts +++ b/src/error-message.ts @@ -12,6 +12,7 @@ export class ErrorMessage { public static EmptyArg = 'Empty function argument.' public static EmptyArray = 'Empty array not allowed.' public static ArrayDimensions = 'Array dimensions are not compatible.' + public static PoolDimensions = 'Range dimensions are not compatible with the window size and the stride.' public static NoSpaceForArrayResult = 'No space for array result.' public static ValueSmall = 'Value too small.' public static ValueLarge = 'Value too large.' diff --git a/src/interpreter/plugin/MatrixPlugin.ts b/src/interpreter/plugin/MatrixPlugin.ts index b2d329d95..c2e553578 100644 --- a/src/interpreter/plugin/MatrixPlugin.ts +++ b/src/interpreter/plugin/MatrixPlugin.ts @@ -9,6 +9,7 @@ import {ErrorMessage} from '../../error-message' import {AstNodeType, ProcedureAst} from '../../parser' import {InterpreterState} from '../InterpreterState' import {InternalScalarValue, InterpreterValue} from '../InterpreterValue' +import {Maybe} from '../../Maybe' import {SimpleRangeValue} from '../../SimpleRangeValue' import {FunctionArgumentType, FunctionPlugin, FunctionPluginTypecheck, ImplementedFunctions} from './FunctionPlugin' @@ -37,6 +38,43 @@ function arraySizeForPoolFunction(inputArray: ArraySize, windowSize: number, str ) } +/** + * Checks whether a square pooling window tiles the input array exactly, so that no window reaches outside of it. + * + * The window has to fit inside the input array and both of its dimensions, reduced by the window size, + * have to be whole multiples of the stride. + * + * @param inputArray - dimensions of the pooled input array + * @param windowSize - side length of the square pooling window + * @param stride - distance between the top-left corners of two consecutive windows + */ +function isPoolWindowFittingInputArray(inputArray: ArraySize, windowSize: number, stride: number): boolean { + return windowSize <= inputArray.width + && windowSize <= inputArray.height + && (inputArray.width - windowSize) % stride === 0 + && (inputArray.height - windowSize) % stride === 0 +} + +/** + * Validates the arguments shared by the pooling functions (MAXPOOL, MEDIANPOOL). + * + * @param matrix - the pooled input range + * @param windowSize - side length of the square pooling window + * @param stride - distance between the top-left corners of two consecutive windows + * @returns a {@link CellError} describing the violated constraint, or `undefined` when the arguments are valid + */ +function poolFunctionArgumentsError(matrix: SimpleRangeValue, windowSize: number, stride: number): Maybe { + if (!matrix.hasOnlyNumbers()) { + return new CellError(ErrorType.VALUE, ErrorMessage.NumberRange) + } + + if (!isPoolWindowFittingInputArray(matrix.size, windowSize, stride)) { + return new CellError(ErrorType.VALUE, ErrorMessage.PoolDimensions) + } + + return undefined +} + export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypecheck { public static implementedFunctions: ImplementedFunctions = { 'MMULT': { @@ -61,8 +99,8 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech sizeOfResultArrayMethod: 'maxpoolArraySize', parameters: [ {argumentType: FunctionArgumentType.RANGE}, - {argumentType: FunctionArgumentType.NUMBER}, - {argumentType: FunctionArgumentType.NUMBER, optionalArg: true}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1, optionalArg: true}, ], vectorizationForbidden: true, }, @@ -71,8 +109,8 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech sizeOfResultArrayMethod: 'medianpoolArraySize', parameters: [ {argumentType: FunctionArgumentType.RANGE}, - {argumentType: FunctionArgumentType.NUMBER}, - {argumentType: FunctionArgumentType.NUMBER, optionalArg: true}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1, optionalArg: true}, ], vectorizationForbidden: true, }, @@ -110,10 +148,19 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech return arraySizeForMultiplication(left, right) } + /** + * Corresponds to MAXPOOL(Range, Window_size, Stride). + * + * Reduces the input range to the maximum value of every window of `Window_size` x `Window_size` cells, + * moving the window by `Stride` cells. The window has to fit inside the range and the range dimensions, + * reduced by the window size, have to be whole multiples of the stride. Otherwise, the function + * returns the #VALUE! error. + */ public maxpool(ast: ProcedureAst, state: InterpreterState): InterpreterValue { return this.runFunction(ast.args, state, this.metadata('MAXPOOL'), (matrix: SimpleRangeValue, windowSize: number, stride: number = windowSize) => { - if (!matrix.hasOnlyNumbers()) { - return new CellError(ErrorType.VALUE, ErrorMessage.NumberRange) + const argumentsError = poolFunctionArgumentsError(matrix, windowSize, stride) + if (argumentsError !== undefined) { + return argumentsError } const outputSize = arraySizeForPoolFunction(matrix.size, windowSize, stride) @@ -133,10 +180,19 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech }) } + /** + * Corresponds to MEDIANPOOL(Range, Window_size, Stride). + * + * Reduces the input range to the median value of every window of `Window_size` x `Window_size` cells, + * moving the window by `Stride` cells. The window has to fit inside the range and the range dimensions, + * reduced by the window size, have to be whole multiples of the stride. Otherwise, the function + * returns the #VALUE! error. + */ public medianpool(ast: ProcedureAst, state: InterpreterState): InterpreterValue { return this.runFunction(ast.args, state, this.metadata('MEDIANPOOL'), (matrix: SimpleRangeValue, windowSize: number, stride: number = windowSize) => { - if (!matrix.hasOnlyNumbers()) { - return new CellError(ErrorType.VALUE, ErrorMessage.NumberRange) + const argumentsError = poolFunctionArgumentsError(matrix, windowSize, stride) + if (argumentsError !== undefined) { + return argumentsError } const outputSize = arraySizeForPoolFunction(matrix.size, windowSize, stride) @@ -227,9 +283,7 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech } } - if (window > array.width || window > array.height - || stride > window - || (array.width - window) % stride !== 0 || (array.height - window) % stride !== 0) { + if (stride > window || !isPoolWindowFittingInputArray(array, window, stride)) { return ArraySize.error() } diff --git a/test/unit/interpreter/matrix-plugin-pooling.spec.ts b/test/unit/interpreter/matrix-plugin-pooling.spec.ts new file mode 100644 index 000000000..f9c57ec8a --- /dev/null +++ b/test/unit/interpreter/matrix-plugin-pooling.spec.ts @@ -0,0 +1,155 @@ +import {CellError, DetailedCellError, ErrorType, HyperFormula} from '../../../src' +import {SimpleCellAddress, simpleCellAddress} from '../../../src/Cell' +import {Config} from '../../../src/Config' +import {ErrorMessage} from '../../../src/error-message' + +const colNumber = (input: string): number => + input.split('').reduce((currentColumn, nextLetter) => currentColumn * 26 + (nextLetter.toUpperCase().charCodeAt(0) - 64), 0) - 1 + +const adr = (stringAddress: string, sheet: number = 0): SimpleCellAddress => { + const result = /^([A-Za-z]+)([0-9]+)$/.exec(stringAddress)! + return simpleCellAddress(sheet, colNumber(result[1]), Number(result[2]) - 1) +} + +const detailedError = (errorType: ErrorType, message?: string): DetailedCellError => + new DetailedCellError(new CellError(errorType, message), new Config().translationPackage.getErrorTranslation(errorType)) + +const poolDimensionsError = () => detailedError(ErrorType.VALUE, ErrorMessage.PoolDimensions) + +const POOL_FUNCTION_NAMES = ['MAXPOOL', 'MEDIANPOOL'] + +const squareRange3x3 = [ + [3, 1, 2], + [9, 7, 8], + [5, 4, 6], +] + +const squareRange4x4 = [ + [1, 2, 10, 20], + [3, 4, 30, 40], + [5, 6, 7, 8], + [7, 8, 9, 10], +] + +const buildEngineWithSheet = (sheetContent: number[][]): { engine: HyperFormula, sheetId: number } => { + const engine = HyperFormula.buildEmpty() + const sheetId = engine.getSheetId(engine.addSheet('Sheet1'))! + engine.setSheetContent(sheetId, sheetContent) + return {engine, sheetId} +} + +// A plain loop instead of `describe.each`, which is not available in the Jasmine-based browser test suite. +POOL_FUNCTION_NAMES.forEach((functionName: string) => describe(`Function ${functionName} dimension validation`, () => { + it('returns an error when the range dimensions are not a whole multiple of the window size', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula(`=${functionName}(A1:C3, 2)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error in a cell when the range dimensions are not a whole multiple of the window size', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + engine.setCellContents(adr('A5', sheetId), [[`=${functionName}(A1:C3, 2)`]]) + + expect(engine.getCellValue(adr('A5', sheetId))).toEqualError(poolDimensionsError()) + }) + + it('returns an error when only one of the range dimensions is a whole multiple of the window size', () => { + const {engine, sheetId} = buildEngineWithSheet([ + [1, 2, 3, 4], + [5, 6, 7, 8], + [9, 10, 11, 12], + ]) + + expect(engine.calculateFormula(`=${functionName}(A1:D3, 2)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error when the window is larger than the range', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula(`=${functionName}(A1:C3, 4)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error when the stride makes the last window reach outside of the range', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula(`=${functionName}(A1:C3, 2, 3)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error when the window size is not a positive integer', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + expect(engine.calculateFormula(`=${functionName}(A1:D4, 0)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, -2)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2.5)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.IntegerExpected)) + }) + + it('returns an error when the stride is not a positive integer', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, 0)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, -1)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, 1.5)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.IntegerExpected)) + }) +})) + +describe('Function MAXPOOL', () => { + it('pools a range whose dimensions are a whole multiple of the window size into maximums', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + expect(engine.calculateFormula('=MAXPOOL(A1:D4, 2)', sheetId)).toEqual([ + [4, 40], + [8, 10], + ]) + }) + + it('pools a range whose dimensions are a whole multiple of a custom stride', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula('=MAXPOOL(A1:C3, 2, 1)', sheetId)).toEqual([ + [9, 8], + [9, 8], + ]) + }) + + it('pools a range with a window covering the whole range', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula('=MAXPOOL(A1:C3, 3)', sheetId)).toEqual(9) + }) + + it('spills the pooled maximums into cells', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + engine.setCellContents(adr('A6', sheetId), [['=MAXPOOL(A1:D4, 2)']]) + + expect(engine.getCellValue(adr('A6', sheetId))).toEqual(4) + expect(engine.getCellValue(adr('B6', sheetId))).toEqual(40) + expect(engine.getCellValue(adr('A7', sheetId))).toEqual(8) + expect(engine.getCellValue(adr('B7', sheetId))).toEqual(10) + }) +}) + +describe('Function MEDIANPOOL', () => { + it('pools a range whose dimensions are a whole multiple of the window size into medians', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + const result = engine.calculateFormula('=MEDIANPOOL(A1:D4, 2)', sheetId) as number[][] + + expect(result[0][0]).toBeCloseTo(2.5) + expect(result[0][1]).toBeCloseTo(25) + expect(result[1][0]).toBeCloseTo(6.5) + expect(result[1][1]).toBeCloseTo(8.5) + }) + + it('spills the pooled medians into cells', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + engine.setCellContents(adr('A6', sheetId), [['=MEDIANPOOL(A1:D4, 2)']]) + + expect(engine.getCellValue(adr('A6', sheetId))).toBeCloseTo(2.5) + expect(engine.getCellValue(adr('B6', sheetId))).toBeCloseTo(25) + expect(engine.getCellValue(adr('A7', sheetId))).toBeCloseTo(6.5) + expect(engine.getCellValue(adr('B7', sheetId))).toBeCloseTo(8.5) + }) +})