1893066 - Fix geomean formula for raptor tests

Status: RESOLVED FIXED

(Testing :: Performance, task, P2)

Description

[Kash Shampur [:kshampur] ⌚EST]

I might be missing some historical context but this current formula is incorrect.

My guess/understanding is the log(i+1) was to avoid log(0) scenarios? however the exp(<result>) -1 to recover the original formula is incorrect? Though i understand using this logarithmic approach since any type of product() (like numpy.prod()) could cause an overflow

we could use scipys geomean?
as we already install scipy for raptor tests here and scipy already handles logarithmic divide error

Comment 1

[Kash Shampur [:kshampur] ⌚EST]

(In reply to Kash Shampur [:kshampur] ⌚EST from comment #0)

I might be missing some historical context but this current formula is incorrect.

My guess/understanding is the log(i+1) was to avoid log(0) scenarios? however the exp(<result>) -1 to recover the original formula is incorrect? Though i understand using this logarithmic approach since any type of product() (like numpy.prod()) could cause an overflow

we could use scipys geomean?
as we already install scipy for raptor tests here and scipy already handles logarithmic divide error

just to add on a bit more, this can easily be verified. the geomean of [8,2] should be 4, however the current implementation would give 4.196152422706632

Comment 2

[Kash Shampur [:kshampur] ⌚EST]

ran a very quick try run. seems fine...

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=3cb2aeffa05de63285cbc7cc0f3652dee5cf6b65&newProject=try&newRevision=376514114f3b792114e01a7831e98a3f4528faf8&page=1&framework=13&showOnlyImportant=1&showOnlyConfident=1

(and with replicates):
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=3cb2aeffa05de63285cbc7cc0f3652dee5cf6b65&newProject=try&newRevision=376514114f3b792114e01a7831e98a3f4528faf8&page=1&framework=13&showOnlyImportant=1&showOnlyConfident=1&replicates=1

I think these are expected

also just realized this exists in talos as well https://searchfox.org/mozilla-central/rev/6121b33709dd80979a6806ff59096a561e348ae8/testing/talos/talos/filter.py#175

Comment 3

[Kash Shampur [:kshampur] ⌚EST]

Just learned it also exists in the standard lib https://docs.python.org/3.8/library/statistics.html#statistics.geometric_mean
only for python >= 3.8. Checking CI it seems all our machines are on >=3.9.
before osx 10.15 was on 3.7 and looks like it is on 3.11 now, after the upgrade from relops.
win/linux are 3.9.x so it will be fine as well (and are slated to be upgraded to 3.11 eventually fwiw)

so maybe using the standard lib over scipy might be better

Comment 4

[Kash Shampur [:kshampur] ⌚EST]

(In reply to Kash Shampur [:kshampur] ⌚EST from comment #3)

Just learned it also exists in the standard lib https://docs.python.org/3.8/library/statistics.html#statistics.geometric_mean
only for python >= 3.8. Checking CI it seems all our machines are on >=3.9.
before osx 10.15 was on 3.7 and looks like it is on 3.11 now, after the upgrade from relops.
win/linux are 3.9.x so it will be fine as well (and are slated to be upgraded to 3.11 eventually fwiw)

so maybe using the standard lib over scipy might be better

To verify this a bit further, I tried generating several non zero random arrays of varying length and compared scipy vs stdlib implementations, because I was concerned of this comment here: No special efforts are made to achieve exact results. (However, this may change in the future.)

I found that they are pretty much the same, and differences that might occur are within numerical tolerance of 1e-17 to 1e-16 so we can treat both implementations as being equivalent

edit forgot to add/check for completeness, the tolerance does depend on the order of magnitude of the elements of the array. The difference of 1e-17 to 1e-16 was for an array with elements between (0,1). If I increase the range to be between (0,10-1000) the difference adjust accordingly as well, up to to 1e-13 on the 1000 end. Just to clarify this is expected floating point error. Eitherway numpy.isclose() is happy with this

Comment 5

[Kash Shampur [:kshampur] ⌚EST]

Attachment: Bug 1893066 - Fix geomean formula for Raptor. r?#perftest

This patch updates the geometric mean formula in Raptor and Talos. The
previous implementation was old and provided only an approximate value.
As of python 3.8, the standard library has it's own implementation and
that is used here for simplicity.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92d53efb67c7
Fix geomean formula for Raptor. r=perftest-reviewers,sparky

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/92d53efb67c7