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

Update cross-media-measurement-api dep for CustomMaximumFrequencyPerUser in DeterministicCount methodology #1462

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

iverson52000
Copy link
Contributor

@iverson52000 iverson52000 commented Feb 5, 2024

@wfa-reviewable
Copy link

This change is Reviewable

@iverson52000 iverson52000 force-pushed the alberthsuu-dynamicclip-api branch from 06f8b2c to 3f71ad1 Compare February 29, 2024 18:10
@iverson52000 iverson52000 changed the title Add Dynamic Clipping methodology for impression measurement Update cross-media-measurement-api dep for CustomMaximumFrequencyPerUser in DeterministicCount methodology Feb 29, 2024
@iverson52000 iverson52000 marked this pull request as ready for review February 29, 2024 18:21
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Shouldn't this also be updating the variance calculation?

Reviewed 5 of 6 files at r1, all commit messages.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @stevenwarejones)

Copy link
Contributor Author

@iverson52000 iverson52000 left a comment

Choose a reason for hiding this comment

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

From what I know, we will be using the same variance formula as McSpecifiedCap with CustomMaximumFrequencyPerUser when they are both using DeterministicCount methodology

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @stevenwarejones)

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Yes, but that needs to happen in this PR.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @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 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @iverson52000)

Copy link
Contributor Author

@iverson52000 iverson52000 left a comment

Choose a reason for hiding this comment

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

Done. Updated variance calculation.

Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @SanjayVas and @stevenwarejones)

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

CC @riemanli

Reviewed 1 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @iverson52000)

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 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @iverson52000)

@iverson52000 iverson52000 force-pushed the alberthsuu-dynamicclip-api branch from b1b418f to 2ac7ec4 Compare March 4, 2024 20:36
Copy link
Contributor Author

@iverson52000 iverson52000 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 6 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @iverson52000)

@iverson52000 iverson52000 merged commit 918b50a into main Mar 4, 2024
4 checks passed
@iverson52000 iverson52000 deleted the alberthsuu-dynamicclip-api branch March 4, 2024 21:28
Copy link
Member

@SanjayVas SanjayVas 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


MODULE.bazel line 139 at r3 (raw file):

bazel_dep(
    name = "cross-media-measurement-api",
    version = "0.60.0",

This jumped straight from 0.58.0 to 0.60.0 without waiting for the intervening PR to handle the changes for 0.59.0. CC @renjiezh

ple13 pushed a commit that referenced this pull request Aug 16, 2024
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.

4 participants