[FIX] functions: cast to matrix #4186
Open
Conversation
This file contains 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
rrahir
reviewed
May 14, 2024
tests/functions/module_math.test.ts
Outdated
| setCellContent(model, "A1", "=SUMIF(CHOOSECOLS(A2,-1),A2)"); | ||
| expect(getEvaluatedCell(model, "A1").value).toBe("#ERROR"); // @compatibility: should be 0 | ||
| expect(getCellError(model, "A1")).toBe( | ||
| "Function SUMIF expects criteria_range to have the same dimension" |
There was a problem hiding this comment.
I don't think this error makes sense on several levels:
- "to have the same dimension" .... as what? -> should be addressed somewhere else.
- Shouldn't we raise the error encapsulated in the args that we go through? In this specific case, we have converted the error arg into an array in the first process step but in the follwing loop, we process the variable
argswhich is not a matrix. Not sure what should be the expected result
There was a problem hiding this comment.
I agree. I wanted to check with @laa-odoo
|
@LucasLefevre are you okay to fixup the suggestion ? |
laa-odoo
force-pushed
the
saas-17.1-visit-simple-arg-lul
branch
3 times, most recently
from
fc43e50
to
6126507
Compare
…ctions Conditional functions like SUMIF using the "visitMatchingRanges" helper did not handle simple arguments correctly. This issue has been fixed, allowing conditional functions to functionally handle errors in a way that is more similar to Excel. Task: 3918882
laa-odoo
force-pushed
the
saas-17.1-visit-simple-arg-lul
branch
from
6126507
to
db08fcd
Compare
…nt size Task: 3918882
Change the CHOOSECOLS and the CHOOSEROWS formulas to search ranges with negative indexs Task: 3918882
Formulas typed as "returning only ranges" can actually return simple values instead of a matrix. This phenomenon occurs precisely when an error is thrown: it is always returned as a simple value. Normally, this type of error is handled during compilation. But with what is said above this is not the case... Two ways to solve the problem have been proposed here: The first solution could have been to encapsulate the throing error in an matrix when it is a function returning a range. By doing this, we ensure that the function typing is correct and we keep the handling of this type of error during compilation. The second solution would be to say that we cannot determine in advance what the result of a sub-function will be (simple error vs array). Thus, the argument check would be done at runtime. The first solution is good, but its implementation might cause comprehension issues for the user. Indeed, it seems strange that for functions rejecting only ranges as input, depending on the return type of the sub-formula (simple vs range), the formula may crash or not for a same error. Let's take the following example to illustrate this problem: Suppose the case of 'SUMIF' which only accepts ranges as parameters. Consider the functions 'ABS', which naturally returns a simple value, and 'TRANSPOSE', which naturally returns a range. - SUMIF(ABS(), 42) - SUMIF(TRANSPOSE(), 42) In these two examples, no arguments were given to 'ABS' and 'TRANSPOSE'. Both sub-functions will respectively return an error. With the first solution, only the first case will throw, whereas in the second case, both will throw. We therefore choose to go with the second solution, despite its implications, to ensure better consistency and understanding for the user. Task: 3918882
Task: 3918882
laa-odoo
force-pushed
the
saas-17.1-visit-simple-arg-lul
branch
from
db08fcd
to
ce19c77
Compare
|
the ref should be done in master |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
=SUMIF(CHOOSECOLS(A1,-1),A2)crashesThis problem actually hides three sub-problems:
1st problem: Conditional functions like SUMIF using the "visitMatchingRanges" helper did not handle simple arguments correctly. This issue has been fixed, allowing conditional functions to functionally handle errors in a way that is more similar to Excel.
2nd problem: The functions CHOOSECOLS and CHOOSEROWS did not take negative indices into account. This is now possible.
3rd problem: Formulas typed as returning only ranges can actually return simple values. This phenomenon occurs precisely when an error is thrown: it is returned as a simple value. Normally, this type of error is handled during compilation.
Concerning the 3rd problem:
Two ways to solve the problem have been proposed here:
The first solution could have been to encapsulate the error in an array when it is a function returning a range. By doing this, we ensure that the function typing is correct and we keep the handling of this type of error during compilation.
The second solution would be to say that we cannot determine in advance what the result of a sub-function will be (simple value/error vs array). Thus, the argument check would be done at runtime.
The first solution is good, but its implementation might cause comprehension issues for the user. Indeed, it seems strange that for functions rejecting only ranges as input, depending on the return type of the sub-formula (simple vs range), the formula may crash or not for the same error.
Let's take the following example to illustrate this problem:
Suppose the case of SUMIF which only accepts ranges as parameters. Consider the functions ABS, which naturally returns a simple value, and TRANSPOSE, which naturally returns a range.
In these two examples, no arguments were given to ABS and TRANSPOSE. Both sub-functions will respectively return an error. With the first solution, only the first case will throw, whereas in the second case, both will throw.
We therefore choose to go with the second solution, despite its implications, to ensure better consistency and understanding for the user.
Task: 3918882
Description:
description of this task, what is implemented and why it is implemented that way.
Task: : 3918882
review checklist