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 wrong assignment in computeMetricVariance for Frequency #1464

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

riemanli
Copy link
Contributor

@riemanli riemanli commented Feb 6, 2024

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

@riemanli
Copy link
Contributor Author

riemanli commented Feb 6, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@riemanli riemanli requested a review from jiayu-google February 6, 2024 01:38
@riemanli riemanli force-pushed the riemanli_fix_frequency_variance branch from 7e46298 to fb6f13f Compare February 6, 2024 02:04
@riemanli riemanli requested a review from chenweiw February 6, 2024 17:25
Copy link
Contributor

@jiayu-google jiayu-google left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @chenweiw and @riemanli)


src/test/kotlin/org/wfanet/measurement/measurementconsumer/stats/VariancesTest.kt line 3354 at r1 (raw file):

    val maximumFrequency = 5
    val relativeFrequencyDistribution =
      (1..maximumFrequency).associateWith { (maximumFrequency - it) / 10.0 }

where is 10.0 from?


val maximumFrequency = 5
val relativeFrequencyDistribution =
(1..maximumFrequency).associateWith { (maximumFrequency - it) / 10.0 }
Copy link

Choose a reason for hiding this comment

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

Line 3354: what does this line mean?

2.582873680435757e+23,
)
.map { it * coefficient }

Copy link

Choose a reason for hiding this comment

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

I'm trying to understand the number of the four lists in line 3388--3409. Where do these expected values come from?

@@ -627,7 +627,7 @@ object VariancesImpl : Variances {
return FrequencyVariances(
relativeVariances = frequencyVariances.relativeVariances.mapValues { coefficient * it.value },
kPlusRelativeVariances =
frequencyVariances.relativeVariances.mapValues { coefficient * it.value },
frequencyVariances.kPlusRelativeVariances.mapValues { coefficient * it.value },
Copy link

Choose a reason for hiding this comment

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

So this addressed the issue that we discussed yesterday -- the stds of rk and rk+ estimations are always equal. Am I right?

Copy link

@chenweiw chenweiw left a comment

Choose a reason for hiding this comment

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

Hi Rieman, I just made a couple of minor comments.

Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @chenweiw and @jiayu-google)


src/main/kotlin/org/wfanet/measurement/measurementconsumer/stats/Variances.kt line 630 at r1 (raw file):

Previously, chenweiw wrote…

So this addressed the issue that we discussed yesterday -- the stds of rk and rk+ estimations are always equal. Am I right?

Yes. The fix here is using the correct field in frequencyVariances


src/test/kotlin/org/wfanet/measurement/measurementconsumer/stats/VariancesTest.kt line 3354 at r1 (raw file):

Previously, jiayu-google wrote…

where is 10.0 from?

10.0 is the normalization term


src/test/kotlin/org/wfanet/measurement/measurementconsumer/stats/VariancesTest.kt line 3354 at r1 (raw file):

Previously, chenweiw wrote…

Line 3354: what does this line mean?

It creates a frequency distribution of [(5 - 1) / 10, (5 - 2) / 10, (5 - 3) / 10, (5 - 4) / 10, (5 - 5) / 10] = [0.4, 0.3, 0.2, 0.1, 0]


src/test/kotlin/org/wfanet/measurement/measurementconsumer/stats/VariancesTest.kt line 3412 at r1 (raw file):

Previously, chenweiw wrote…

I'm trying to understand the number of the four lists in line 3388--3409. Where do these expected values come from?

These values are generated from the Colab notebook you and Jiayu wrote. Basically, I use the notebook as a ground truth generator to ensure the code in prod works the same as the code in the Colab notebook.

@SanjayVas SanjayVas mentioned this pull request Feb 6, 2024
15 tasks
Copy link

@chenweiw chenweiw left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiayu-google)

Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiayu-google and @Marco-Premier)


src/test/kotlin/org/wfanet/measurement/measurementconsumer/stats/VariancesTest.kt line 3354 at r1 (raw file):

Previously, riemanli (Rieman) wrote…

10.0 is the normalization term

It creates a frequency distribution of [(5 - 1) / 10, (5 - 2) / 10, (5 - 3) / 10, (5 - 4) / 10, (5 - 5) / 10] = [0.4, 0.3, 0.2, 0.1, 0]

Copy link
Contributor

@Marco-Premier Marco-Premier left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiayu-google and @stevenwarejones)

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiayu-google)

Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Dismissed @jiayu-google from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jiayu-google)

@riemanli riemanli force-pushed the riemanli_fix_frequency_variance branch from fb6f13f to 789d050 Compare February 12, 2024 17:33
@riemanli riemanli enabled auto-merge (squash) February 12, 2024 17:33
Copy link
Contributor

@jiayu-google jiayu-google left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @riemanli)

@riemanli riemanli merged commit e82d082 into main Feb 12, 2024
5 checks passed
@riemanli riemanli deleted the riemanli_fix_frequency_variance branch February 12, 2024 21:17
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.

6 participants