feature(geomean): add geomean function

[prabhathc]

Add geomean function, closes#6152!

[nicolaskruchten]

@alexcjohnson should this be supported in other places like histfunc etc?

[acxz]

@nicolaskruchten is that referring to just geomean or the entire categoryorder attribute?

[nicolaskruchten]

geomean: all the other aggregation functions you can sort by (sum, mean, median) are also supported in a few other places like histfunc, so it would be nice to keep that parallelism going if possible.

[acxz]

I'd be down to tackle that in another PR if you can list where else to add the geomean statistic.

[archmoj]

Wondering geomean may not be the best naming as it could be point to our geo subplots.
How do you feel about geometric mean or g-mean alternatives?
Also there is harmonic mean which one may want to add. Right?
In that case something like h-mean works for me; but harmean does not :)

[archmoj]

On another note, we are wondering why the automatic CircleCI tests for this PR are not triggered?!
Do you use any special flags when pushing?

[acxz]

yea, harmean doesn't sound so nice.
I don't particularly like g-mean either since it may not be obvious. I'd actually rather pref geometric_mean over g-mean.

edit: a quick goog/bing search shows the wikipedia article of Geometric mean with a search query of g-mean. I suppose, it is descriptive enough.

[acxz]

On another note, we are wondering why the automatic CircleCI tests for this PR are not triggered?!

I was gonna ask you guys the same thing lol.
There were commits where the CI was triggered, but I believe this stopped when we made this PR a draft and then undid that action.

[acxz]

I can try rebasing these commits on master and force pushing to see if that triggers the CI?

[archmoj]

I can try rebasing these commits on master and force pushing to see if that triggers the CI?

Force pushing is discouraged on open pull requests as it could confuse reviewers.
And your branch is only 11 commits behind upstream master. So that shouldn't be the reason.
Let's see next time you push a commit into this PR, that may trigger the tests to run on CircleCI.

[archmoj]

On another note, we are wondering why the automatic CircleCI tests for this PR are not triggered?!

I was gonna ask you guys the same thing lol. There were commits where the CI was triggered, but I believe this stopped when we made this PR a draft and then undid that action.

@antoinerg any idea how we could trigger CI runs on this pull?

[alexcjohnson]

Wondering geomean may not be the best naming as it could be point to our geo subplots.

I see your point, though I think in context it's pretty clear. Looking around I see gmean used in scipy.stats and various npm packages, geomean used in MATLAB and Excel, and geometric_mean used in the Python stdlib statistics package. I'm comfortable leaving it as geomean but @archmoj if you feel strongly about it I'd be OK with either gmean or geometric mean (with a space, not a dash or underscore, so the full values would be geometric mean ascending or geometric mean descending)

Harmonic mean commonly refers to the inverse of the mean of inverses (see eg npm and python stdlib) so let's stay away from that. But given that we all agree harmean is unpleasant, so if we ever did add it we'd need to use hmean or harmonic mean, perhaps that argues against geomean for consistency? In that case I'm leaning toward geometric mean, clarity over terseness.

should this be supported in other places like histfunc etc?

Yes, that would be nice. I only see two more places we could add this, and one of them (the aggregate transform) is deprecated so I'm happy to ignore that. So histfunc (attribute values, bin functions, which get applied here) is really the only place this needs to be added. @nicolaskruchten am I missing anything?

Two frustrating things though:

(1) we weren't consistent about the naming - for histfunc and the aggregate transform we said avg rather than mean. I would suggest that we add mean as a synonym for avg and maybe eventually deprecate avg, then add geometric mean alongside this.

(2) The implementation for histfunc is different from that used in category ordering, in that it loops over the array only once, calculating all the quantities it needs for all the bins simultaneously (ie sums and counts), then in a separate step it loops over all the bins to complete the calculation (divide each sum by each count).

[archmoj]

I'd vote for geometric mean too, when there is an election here :)

[nicolaskruchten]

Two frustrating things though

Ah yeah I remember these. Well, probably reason enough not to do extra work here then :)

[acxz]

@archmoj I'm not sure how to fix the following error: https://app.circleci.com/pipelines/github/plotly/plotly.js/10836/workflows/443b723f-0ded-4052-9189-1977b51761ae/jobs/238687?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link&utm_content=summary

9) takes the geometric mean of all values per category across traces of type ohlc
calculated data and points category ordering by value
Error: Expected $[1][1] = 2.82842712474619 to equal 2.8284271247461903.
at <Jasmine>
at /tmp/test/jasmine/tests/calcdata_test.js:1057:1 <- /tmp/8028cfc1362ed8eb2b52841f495d61eb.browserify.js:283624:41
10) takes the geometric mean of all values per category across traces of type candlestick
calculated data and points category ordering by value
Error: Expected $[1][1] = 2.82842712474619 to equal 2.8284271247461903.
at <Jasmine>
at /tmp/test/jasmine/tests/calcdata_test.js:1057:1 <- /tmp/8028cfc1362ed8eb2b52841f495d61eb.browserify.js:283624:41

Also if you could tell me how to run just a specific test locally that would be great!

[archmoj]

@prabhathc Could you please fetch upstream/master and merge master into this branch to resolve the conflicts?

[acxz]

@archmoj done, merge caused the geometric mean name to be single quoted unlike the others in aggFn. Please confirm if that is okay in terms of code style.

Note: the same test failure from 2 years ago still occurs, that I'm still not sure how to resolve
any way to relax the tolerance on the equality maybe?

[archmoj]

To reflect the changes to the API, please run

npm run schema && git add test/plot-schema.json && git commit -m "update plot-schema diff"

[acxz]

@archmoj done, all the tests finally pass!
Thanks for your help in pushing this PR through!

[archmoj]

Nicely done.
💃
Thank you!