Jk/subtotal #536

izulin · 2020-10-04T13:23:29Z

Context

14 new functions: VAR.P, VAR.S, VARA, VARPA, STDEV.P, STDEV.S, STDEVA, STDEVPA, SUBTOTAL, PRODUCT, VARP, VAR, STDEVP, STDEV.

How has this been tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature or improvement (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Additional language file or change to the existing one (translations)

Related issue(s):

Checklist:

My code follows the code style of this project,
My change requires a change to the documentation,
I described the modification in the CHANGELOG.md file.

CHANGELOG.md

Co-authored-by: Jakub Kowalski <[email protected]>

wojciechczerniak

We should add to https://github.com/handsontable/hyperformula/blob/master/docs/guide/known-limitations.md that SUBTOTAL is not supported fully. There is no difference between 1 and 101. We don't have support for row/column visibility meta data. @izulin @voodoo11 is this even possible? A callback in options maybe?

hfInstance = new Hyperformula({
  isHidden: function(rowIndex) {
    return handsontableInstance.isHidden(rowIndex);
  }
});

src/i18n/languages/csCZ.ts

test/interpreter/function-subtotal.spec.ts

CHANGELOG.md

docs/guide/built-in-functions.md

test/interpreter/function-vars.spec.ts

test/interpreter/function-vara.spec.ts

test/interpreter/function-stdevs.spec.ts

test/interpreter/function-stdevpa.spec.ts

test/interpreter/function-stdevp.spec.ts

wojciechczerniak · 2020-10-05T11:40:24Z

test/interpreter/function-stdeva.spec.ts

+    expect(engine.getCellValue(adr('A1'))).toBeCloseTo(0.707106781186548, 6)
+    expect(engine.getCellValue(adr('A2'))).toBeCloseTo(1.78885438199983, 6)
+  })
+})


https://docs.oasis-open.org/office/OpenDocument/v1.3/csprd02/part4-formula/OpenDocument-v1.3-csprd02-part4-formula.html#__RefHeading__1018768_715980110

Unlike the STDEV function includes values of type Text and Logical. Text values are treated as number 0. Logical TRUE is treated as 1, and FALSE is treated as 0. Empty cells are not included.

The handling of string constants as parameters is implementation-defined. Either, string constants are converted to numbers, if possible and otherwise, they are treated as 0, or string constants are always treated as 0.

🙈 Which implementation did we choose? How does it relate to Excel requirements?

Arguments can be the following: numbers; names, arrays, or references that contain numbers; text representations of numbers; or logical values, such as TRUE and FALSE, in a reference.

Arguments that contain TRUE evaluate as 1; arguments that contain text or FALSE evaluate as 0 (zero).

If an argument is an array or reference, only values in that array or reference are used. Empty cells and text values in the array or reference are ignored.

Arguments that are error values or text that cannot be translated into numbers cause errors.

If you do not want to include logical values and text representations of numbers in a reference as part of the calculation, use the STDEV function.

Source: https://docs.oasis-open.org/office/OpenDocument/v1.3/csprd02/part4-formula/OpenDocument-v1.3-csprd02-part4-formula.html#__RefHeading__1018768_715980110

BTW We should add more tests for the requirements above

Which implementation did we choose? How does it relate to Excel requirements?

I don't really understand -- we implemented STDEVA function.

The logic behind -A variants are nothing new, we already handled it with AVERAGEA, MAXA, MINA, SUMA functions. I'll add more extensive test.

ODFF description suggests that STDEV can have different behavior depending on the implementation: "The handling of string constants as parameters is implementation-defined. Either, string constants are converted to numbers, if possible and otherwise, they are treated as 0, or string constants are always treated as 0."

I see. Then for consistency I would keep it the same as in AVERAGEA. Where should it be documented?

I would create a test that is our behaviour documentation with a clear description and an inconsistency comment

izulin · 2020-10-06T10:44:43Z

We should add to https://github.com/handsontable/hyperformula/blob/master/docs/guide/known-limitations.md that SUBTOTAL is not supported fully. There is no difference between 1 and 101. We don't have support for row/column visibility meta data. @izulin @voodoo11 is this even possible? A callback in options maybe?
hfInstance = new Hyperformula({
  isHidden: function(rowIndex) {
    return handsontableInstance.isHidden(rowIndex);
  }
});

That would be super messy with the logic behind triggering recalculations, caching etc. (if done correctly, would probably result in performance hit).

CHANGELOG.md

bardek8

Thanks for the fixes!
LGTM, but let's wait for a green light from @wojciechczerniak

wojciechczerniak

I've added few examples but all tests should be reviewed if they can be improved.

wojciechczerniak · 2020-10-14T15:37:16Z

test/interpreter/function-vars.spec.ts

+  it('should calculate variance (sample)', () => {
+    const engine = HyperFormula.buildFromArray([
+      ['=VAR.S(2, 3)'],
+      ['=VAR.S(2, 3, 4, TRUE(), FALSE(), "1",)'],
+      ['=VAR.S(B3:I3)', 2, 3, 4, true, false, 'a', '\'1', null],
+    ])
+    expect(engine.getCellValue(adr('A1'))).toEqual(0.5)
+    expect(engine.getCellValue(adr('A2'))).toBeCloseTo(2.28571428571429, 6) //inconsistency with product #1
+    expect(engine.getCellValue(adr('A3'))).toEqual(1)
+  })


Please add clear descriptions to the conditions we're checking

it('should ignore empty cells, logical values, text and error values if argument is a reference', () => { ['=VAR.S(B3:I3)', 2, 3, 4, true, false, 'a', '\'1', null, '#DIV/0'],

it('should count logical values and text representation of numbers that are arguments', () => { ['=VAR.S(2, 3, 4, TRUE(), FALSE(), "1",)'],

Errors are not ignored.

Inconsistency with the docs then? Not again...

Where is it in the docs?

"Empty cells, logical values, text, or error values in the array or reference are ignored." Source: https://support.microsoft.com/en-us/office/var-s-function-913633de-136b-449d-813e-65a00b2b990b

Well, the implementation clearly propagates errors 🥇

Ok, nothing to add here then 🤷 Thanks for checking

test/interpreter/function-varpa.spec.ts

test/interpreter/function-varp.spec.ts

test/interpreter/function-vara.spec.ts

wojciechczerniak

Awesome! Few descriptions can be more specific though. They say the same but results are different. I've added specifics to two of them but could you apply those to all pairs?
STDEVA vs STDEVS
STDEVPA vs STDEVP
VARA vs VARS
VARPA vs VARP

docs/guide/known-limitations.md

test/interpreter/function-varpa.spec.ts

test/interpreter/function-varp.spec.ts

test/interpreter/function-vara.spec.ts

test/interpreter/function-vars.spec.ts

Co-authored-by: Wojciech Czerniak <[email protected]>

wojciechczerniak · 2020-10-14T18:57:57Z

src/interpreter/plugin/NumericAggregationPlugin.ts

+    'VAR.S': {
+      method: 'vars',
+    },
+    'VAR.P': {
+      method: 'varp',
+    },


Would be a valid solution to make VARP and VARS aliases here that are linking to the same method but under different name?

Suggested change

'VAR.S': {

method: 'vars',

},

'VAR.P': {

method: 'varp',

},

'VAR.S': {

method: 'vars',

},

'VAR.P': {

method: 'varp',

},

'VARS': {

method: 'vars',

},

'VARP': {

method: 'varp',

},

BTW. Are we missing arguments definitions or they are redundant?

those functions are not using the mechanism that requires args definitions

There is no such method as 'VARS' -- VAR should be an alias of VAR.S

Ok, we have a problem on our list
https://github.com/handsontable/formula-parser/blob/d056ea2351153b77831b9a92842ef198b3f517f0/src/supported-formulas.js#L371-L376

wojciechczerniak

Great! 🥇

voodoo11 and others added 18 commits October 2, 2020 13:02

Function SUBTOTAL init

dc51600

Simply use implementations of AVERAGE, SUM, COUNT, COUNTA, MAX, MIN

9dd7ab6

Sort translations

abdfd0c

Function PRODUCT

4c81061

function implementations

60149f7

translations

41ee6d2

translations

c209d65

.

6a1b114

translations

2efeb98

docs

f9edac7

linter

6b94282

fixed translation

dc35208

a-versions

b54a889

.

01362bb

.

cd1cd17

.

6dc222a

.

04f53bb

docs

8590b38

izulin requested review from bardek8 and wojciechczerniak October 4, 2020 13:24

Merge branch 'develop' into jk/subtotal

b54126a

voodoo11 reviewed Oct 4, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

izulin and others added 2 commits October 4, 2020 15:34

Update CHANGELOG.md

8ed9107

Co-authored-by: Jakub Kowalski <[email protected]>

Remove TODO

c722b94

voodoo11 approved these changes Oct 5, 2020

View reviewed changes

wojciechczerniak suggested changes Oct 5, 2020

View reviewed changes

izulin added 2 commits October 6, 2020 11:50

docs

c236f13

tests

2aa3033

Update known-limitations.md

172f8d0

.

3a3727e

izulin requested a review from bardek8 October 9, 2020 12:38

linter

4403788

izulin commented Oct 10, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

1b9468e

bardek8 approved these changes Oct 10, 2020

View reviewed changes

izulin and others added 2 commits October 12, 2020 20:02

Merge branch 'develop' into jk/subtotal

1d51f1e

merge

933ba3e

wojciechczerniak suggested changes Oct 14, 2020

View reviewed changes

tests descriptions

569a152

izulin requested a review from wojciechczerniak October 14, 2020 17:42

Merge branch 'develop' into jk/subtotal

bb327b7

wojciechczerniak suggested changes Oct 14, 2020

View reviewed changes

izulin and others added 5 commits October 14, 2020 20:49

Update docs/guide/known-limitations.md

25e1a39

Co-authored-by: Wojciech Czerniak <[email protected]>

Update test/interpreter/function-varpa.spec.ts

1e141b2

Co-authored-by: Wojciech Czerniak <[email protected]>

Update test/interpreter/function-varp.spec.ts

7adf577

Co-authored-by: Wojciech Czerniak <[email protected]>

Update test/interpreter/function-vars.spec.ts

c882391

Co-authored-by: Wojciech Czerniak <[email protected]>

Update test/interpreter/function-vara.spec.ts

eeb69ce

Co-authored-by: Wojciech Czerniak <[email protected]>

wojciechczerniak reviewed Oct 14, 2020

View reviewed changes

izulin added 4 commits October 14, 2020 21:08

.

faf1645

aliases

601f5b6

docs

9d3c779

tests for aliases

b271ea4

izulin requested a review from wojciechczerniak October 14, 2020 20:14

wojciechczerniak approved these changes Oct 14, 2020

View reviewed changes

izulin merged commit 719b630 into develop Oct 14, 2020

izulin deleted the jk/subtotal branch October 14, 2020 20:40

This was referenced Oct 22, 2020

Add Math and Trigonometry functions #161

Closed

Add Compatibility functions #152

Closed

Add Statistical functions #160

Closed

Jk/subtotal #536

Jk/subtotal #536

Uh oh!

Conversation

izulin commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

How has this been tested?

Types of changes

Related issue(s):

Checklist:

Uh oh!

Uh oh!

wojciechczerniak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izulin Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izulin commented Oct 6, 2020

Uh oh!

Uh oh!

bardek8 left a comment

Choose a reason for hiding this comment

Uh oh!

wojciechczerniak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojciechczerniak Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wojciechczerniak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

izulin commented Oct 4, 2020 •

edited

Loading

izulin Oct 6, 2020 •

edited

Loading

wojciechczerniak Oct 14, 2020 •

edited

Loading