[FIX] evaluation: fix operation with empty matrices #6930
Conversation
Task Description When using empty matrices for math operation (multiplication or inversion), as in the FORECAST formula, we got a traceback and the error message was not clear for the user. This commits aims to fix it by checking that the matrices used in theses operations are not empty and returning an understandable error if it's not the case. Related Task Task: 5001658
|
|
| if (M.length < 1 || M[0].length < 1) { | ||
| throw new Error(_t("Function [[FUNCTION_NAME]] invert matrix error, empty matrix")); |
There was a problem hiding this comment.
It is impossible for a user to provide an empty matrix.
There was a problem hiding this comment.
It was, at first, as the invertMatrix function is called in other part of the code than the INVERT_MATRIX formula (it's also used in the getLMS method). But as we first try to multiply two matrices before trying to invert the result, indeed it may be not needed anymore.
Shouldn't we let this part "in case of" one day we use this method in other part of the code, or keeping thins as a safety guard is not a good idea ?
There was a problem hiding this comment.
we can keep the check, but remove the translation (it's useless work for translators if the string is not user-facing)
| setCellContent(model, "A1", "=SLOPE(C2:C3, B2:B3)"); | ||
| expect(getEvaluatedCell(model, "A1").value).toBe("#ERROR"); | ||
| expect((getEvaluatedCell(model, "A1") as ErrorCell).error.message).toBe( | ||
| "Cannot multiply matrices: empty matrices cannot be multiplied." |
There was a problem hiding this comment.
I think the arguments should be validated in the compute with a proper error message telling which argument is faulty. Here the message is quite useless: it doesn't tell you which arg is wrong, and at first glance you're trying to compute the slope, but the error is mentioning matrix multiplication.
| @@ -30,6 +33,10 @@ export function invertMatrix(M: Matrix<number>): { | |||
| // (b) Multiply a row by a scalar. This multiply the determinant by that scalar. | |||
| // (c) Add to a row a multiple of another row. This does not change the determinant. | |||
|
|
|||
| if (M.length < 1 || M[0].length < 1) { | |||
| throw new Error(_t("Function [[FUNCTION_NAME]] invert matrix error, empty matrix")); | |||
There was a problem hiding this comment.
user-facing errors should be EvaluationError
| throw new Error(_t("Function [[FUNCTION_NAME]] invert matrix error, empty matrix")); | |
| throw new EvaluationError(_t("Function [[FUNCTION_NAME]] invert matrix error, empty matrix")); |
Task Description
When using empty matrices for math operation (multiplication or inversion), as in the FORECAST formula, we got a traceback and the error message was not clear for the user. This commits aims to fix it by checking that the matrices used in theses operations are not empty and returning an understandable error if it's not the case.
Related Task
review checklist