Skip to content
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

[FIX] evaluation: accept 1x1 result array in sub-formula #3750

Closed
wants to merge 1 commit into from

Conversation

laa-odoo
Copy link
Collaborator

Description:

Before this commit, when a formula function
expected a simple argument, and it received
an array, we transformed the array into a
simple argument if the array was of size 1x1

However, this process was only done when
reading the references to the ranges.

This commit extends the process when the result
array comes from a sub-formula and no longer
range references.

Task: : 3756474

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Feb 27, 2024

@laa-odoo
Copy link
Collaborator Author

Possibility to go further and remove refFn in compilation parameters.
Since we treat the form of the arguments (matrix or not) before each
sub-formula, --> we can remove the code doing the same thing across
reference reading

POC #3747

Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few performance suggestions.
Did you try measuring if it has a measurable impact?

src/functions/index.ts Outdated Show resolved Hide resolved
src/functions/index.ts Outdated Show resolved Hide resolved
@laa-odoo laa-odoo force-pushed the 17.0-fix-evaluation-type-guard-laa branch from de45464 to 5c4af69 Compare February 29, 2024 10:22
Comment on lines 90 to 91
const argDefinition = descr.args[descr.getArgToFocus(i + 1) - 1];
const acceptMatrix = argDefinition.type.some((t) => t.startsWith("RANGE"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can measure a performance hit of ~+4.5% (on RNG's spreadsheet)
maybe we can try to move those two lines to check if the arg accepts a matrix outside of return function (this: EvalContext, ... to compute it only once (keep an array of booleans)

Copy link
Collaborator Author

@laa-odoo laa-odoo Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay with this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LucasLefevre

to know if arg accept matrix or not, an attribute was added on argFunctionDecription.
The attribute is computed when formula is added to the formula registry.

If ok for you, I fixup the last commit

src/functions/index.ts Outdated Show resolved Hide resolved
@laa-odoo laa-odoo force-pushed the 17.0-fix-evaluation-type-guard-laa branch from 5c4af69 to 116228e Compare March 14, 2024 10:24
Before this commit, when a formula function
expected a simple argument, and it received
an array, we transformed the array into a
simple argument if the array was of size 1x1

However, this process was only done when
reading the references to the ranges.

This commit extends the process when the result
array comes from a sub-formula and no longer
range references.

Task 3756474
@LucasLefevre LucasLefevre force-pushed the 17.0-fix-evaluation-type-guard-laa branch from 116228e to b113dfb Compare March 15, 2024 12:57
Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't measure any performance impact 👍
Thanks
robodoo r+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants