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

Add custom_maximum_frequency_per_user field into DeterministicCount methodology #197

Merged
merged 4 commits into from
Mar 4, 2024

Conversation

iverson52000
Copy link
Contributor

@iverson52000 iverson52000 commented Feb 1, 2024

  • Add custom_maximum_frequency_per_user field for EDP to report back the maximumFrequencyCap they use for impression measurement
  • Add documentation that if EDP specify the new field for impression, the maximum_frequency_per_user in measurement_spec will be ignored

@wfa-reviewable
Copy link

This change is Reviewable

@iverson52000 iverson52000 force-pushed the alberthsuu-dynamicclip-impression branch 2 times, most recently from 76b9cd3 to 65181dd Compare February 1, 2024 19:27
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.

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @iverson52000, @kungfucraig, and @riemanli)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 92 at r2 (raw file):

// Parameters used when applying the Dynamic Clipping count methodology.
message DynamicClipCount {
  // Optimized maximum frequency per user value calculated by the EDP.

We don't use the term EDP in the API. Instead we refer to the DataProvider resource

Suggestion:

`DataProvider`

src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 93 at r2 (raw file):

message DynamicClipCount {
  // Optimized maximum frequency per user value calculated by the EDP.
  int32 maximum_frequency_per_user = 1;

Indicate that this is required

Suggestion:

int32 maximum_frequency_per_user = 1 [(google.api.field_behavior) = REQUIRED];

src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 97 at r2 (raw file):

    // Maximum frequency per user that will be included in this measurement.
    //
    // Required and validated at Kingdom. It could be ignored by EDP if EDP uses

Drop this comment in favor of a field behavior annotation ([(google.api.field_behavior) = REQUIRED])

Code quote:

Required and validated at Kingdom. 

src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 97 at r2 (raw file):

    // Maximum frequency per user that will be included in this measurement.
    //
    // Required and validated at Kingdom. It could be ignored by EDP if EDP uses

Same note here re: EDP

Code quote:

EDP

src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 99 at r2 (raw file):

    // Required and validated at Kingdom. It could be ignored by EDP if EDP uses
    // Dynamic Clipping methodology to calculate an optimized value and report
    // back to Kingdom.

Don't reference the Kingdom here and instead specifically reference the relevant parts of the API

e.g. "If the DynamicClipCount methodology is used, this field will be ignored and the value from that message will be used instead."

Code quote:

    // Dynamic Clipping methodology to calculate an optimized value and report
    // back to Kingdom.

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.

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @kungfucraig, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 92 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

We don't use the term EDP in the API. Instead we refer to the DataProvider resource

Done.


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 93 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Indicate that this is required

Done.


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 97 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Same note here re: EDP

Done.


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 97 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Drop this comment in favor of a field behavior annotation ([(google.api.field_behavior) = REQUIRED])

Done.


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 99 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Don't reference the Kingdom here and instead specifically reference the relevant parts of the API

e.g. "If the DynamicClipCount methodology is used, this field will be ignored and the value from that message will be used instead."

Done.

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.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kungfucraig and @riemanli)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kungfucraig and @riemanli)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 101 at r3 (raw file):

    // instead.
    int32 maximum_frequency_per_user = 2
        [(google.api.field_behavior) = REQUIRED];

FYI for future reviewers: this field has always been required. This PR is just fixing the documentation of that fact.

Code quote:

(google.api.field_behavior) = REQUIRED

Copy link
Contributor

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

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

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kungfucraig, @Marco-Premier, and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 101 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

FYI for future reviewers: this field has always been required. This PR is just fixing the documentation of that fact.

Thanks for the clarification!

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1, 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier and @stevenwarejones)

Copy link
Contributor

@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 1 of 4 files at r1, 1 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @iverson52000 and @Marco-Premier)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

        DeterministicCount deterministic_count = 4;

        // Dynamic Clipping count methodology.

Is dynamic clip count a methodology? Can it be done in a non-deterministic manner with a custom variance? If so, then it should be a new field rather than inside of methodology.

Also, it doesn't have to be dynamic, either. For example, an EDP might have a requirement of a different constant clipping value than what the MC provides.

@SanjayVas SanjayVas requested a review from kungfucraig February 5, 2024 20:22
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 (waiting on @kungfucraig and @Marco-Premier)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

an EDP might have a requirement of a different constant clipping value than what the MC provides.

I'm not sure we care about or want to support this use case. @kungfucraig ?

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.

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


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

an EDP might have a requirement of a different constant clipping value than what the MC provides.

I'm not sure we care about or want to support this use case. @kungfucraig ?

Hi Steven. If I understand your question correctly, the new DynamicClipCount methodology is EDP uses the DynamicCliping library to compute the noised impression value and optimized threshold(maximum_frequency_per_user) and report back to Kingdom. In this case, it’s considered as a methodology since it’s different from other ones, ex. DeterministicCount which uses fixed cap passed from MC?

Copy link
Member

@kungfucraig kungfucraig 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 @Marco-Premier and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Hi Steven. If I understand your question correctly, the new DynamicClipCount methodology is EDP uses the DynamicCliping library to compute the noised impression value and optimized threshold(maximum_frequency_per_user) and report back to Kingdom. In this case, it’s considered as a methodology since it’s different from other ones, ex. DeterministicCount which uses fixed cap passed from MC?

@stevenwarejones I'm not exactly sure what you're proposing, can you explain a bit?

My sense is that keeping DC on its own allows the system to infer some things about it automatically, which allows EDPs to offload those calculations to Halo.

If we capture it more generally it'll mean more work for EDPs I think.

Copy link
Contributor

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @iverson52000, @kungfucraig, @Marco-Premier, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

@stevenwarejones I'm not exactly sure what you're proposing, can you explain a bit?

My sense is that keeping DC on its own allows the system to infer some things about it automatically, which allows EDPs to offload those calculations to Halo.

If we capture it more generally it'll mean more work for EDPs I think.

@kungfucraig - the dynamic clipping value has to be calculated by the EDP, per my understanding.
@iverson52000 , I see the following scenarios:

  1. EDP calculates dynamic clipping to maximize utility vs privacy budget
  2. EDP uses a different universal max than what the kingdom provides, so it uses that value
    Once Custom Clipping is set, the actual counting might not be deterministic, right? Someone might use a sketch or a deterministic count or some other unknown methodology in the future. For this reason, I don't see custom clipping as a counting methodology.

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.

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


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

@kungfucraig - the dynamic clipping value has to be calculated by the EDP, per my understanding.
@iverson52000 , I see the following scenarios:

  1. EDP calculates dynamic clipping to maximize utility vs privacy budget
  2. EDP uses a different universal max than what the kingdom provides, so it uses that value
    Once Custom Clipping is set, the actual counting might not be deterministic, right? Someone might use a sketch or a deterministic count or some other unknown methodology in the future. For this reason, I don't see custom clipping as a counting methodology.

My answer to your questions:

  1. Yes that sounds right
  2. The “universal max” here, do you mean the maximum_frequency_per_user in measurement_spec? This value will not be used for DynamicClipCount methodology. “deterministic” here if you mean noise, the Dynamic Clipping library adds noise to the input frequency histogram in order to find the optimized threshold. And currently the library is designed to take VID frequency histogram as input instead of sketch so that’s why it’s described as counting methodology. Do I understand it correctly?

Will let @jiayu-google and @chenweiw to add more clarity on the actual algorithm

Copy link

@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: all files reviewed, 1 unresolved discussion (waiting on @chenweiw and @Marco-Premier)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

My answer to your questions:

  1. Yes that sounds right
  2. The “universal max” here, do you mean the maximum_frequency_per_user in measurement_spec? This value will not be used for DynamicClipCount methodology. “deterministic” here if you mean noise, the Dynamic Clipping library adds noise to the input frequency histogram in order to find the optimized threshold. And currently the library is designed to take VID frequency histogram as input instead of sketch so that’s why it’s described as counting methodology. Do I understand it correctly?

Will let @jiayu-google and @chenweiw to add more clarity on the actual algorithm

Hi @stevenwarejones , here are some thoughts:
DynamicClipCount here means DeterministicCount + DynamicClip. (While DeterministicCount here means DeterministicCount + MCSpecifiedClip.)

The clipping methodology is orthogonal to the counting methodology, so one option is to separate the messages of clipping and counting methodologies.

Albert's codes do not separate the clipping and counting methodologies. I think it's a reasonable approach for now, as we only support the deterministic counting --- we don't support LiquidLegions or other counting. If later we do not want support LiquidLegions counting, it makes sense to separate the messages of clipping and counting methodologies.

Copy link

@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: all files reviewed, 1 unresolved discussion (waiting on @chenweiw and @Marco-Premier)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, jiayu-google wrote…

Hi @stevenwarejones , here are some thoughts:
DynamicClipCount here means DeterministicCount + DynamicClip. (While DeterministicCount here means DeterministicCount + MCSpecifiedClip.)

The clipping methodology is orthogonal to the counting methodology, so one option is to separate the messages of clipping and counting methodologies.

Albert's codes do not separate the clipping and counting methodologies. I think it's a reasonable approach for now, as we only support the deterministic counting --- we don't support LiquidLegions or other counting. If later we do not want support LiquidLegions counting, it makes sense to separate the messages of clipping and counting methodologies.

Sorry typo:
If later we do not want support LiquidLegions counting -> If later we DO want support LiquidLegions counting

Copy link
Contributor

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

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


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, jiayu-google wrote…

Sorry typo:
If later we do not want support LiquidLegions counting -> If later we DO want support LiquidLegions counting

@jiayu-google - I think we're roughly on the same page here. Two more thoughts:

  1. I'm not a fan of conflating two different orthogonal methodologies. Can we have two fields: counting_methodology and privacy_mechanisms (that would include maximum clipping value)
  2. For MCSpecifiedClip, where is this specified in the Measurement.Result? I don't think we support this today. Assuming, we don't support it today, it could also be under privacy_mechanisms

Copy link
Member

@kungfucraig kungfucraig 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 @chenweiw and @Marco-Premier)


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

@jiayu-google - I think we're roughly on the same page here. Two more thoughts:

  1. I'm not a fan of conflating two different orthogonal methodologies. Can we have two fields: counting_methodology and privacy_mechanisms (that would include maximum clipping value)
  2. For MCSpecifiedClip, where is this specified in the Measurement.Result? I don't think we support this today. Assuming, we don't support it today, it could also be under privacy_mechanisms

@stevenwarejones Thanks for clarifying. I understand your point and agree. Dynamic Clipping is not a counting methodology.

Maybe we add a field to the existing counting mechanisms where dynamic clipping could apply (e.g. DeterministicCount, but perhaps in the future LiquidLegions) for EDPs to provide the clipped frequency value?

Copy link
Contributor

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

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


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

@stevenwarejones Thanks for clarifying. I understand your point and agree. Dynamic Clipping is not a counting methodology.

Maybe we add a field to the existing counting mechanisms where dynamic clipping could apply (e.g. DeterministicCount, but perhaps in the future LiquidLegions) for EDPs to provide the clipped frequency value?

i'd be okay with adding it to the existing field for DeterministicCount - its not perfect but good enough for me

@iverson52000 iverson52000 force-pushed the alberthsuu-dynamicclip-impression branch 2 times, most recently from c49183c to b1ff1b5 Compare February 6, 2024 23:18
@iverson52000 iverson52000 force-pushed the alberthsuu-dynamicclip-impression branch from d7c2e9c to 0f6ae28 Compare February 8, 2024 18:01
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.

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @chenweiw, @jiayu-google, @Marco-Premier, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 93 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Let's not copy the field in more than on place. Just have dynamic_clipping_maximum_frequency_per_user or something and indicate that it's only specified when dynamic clipping is used. We can always wrap in a oneof later if we need more methodologies for computing maximum frequency.

Done.


src/main/proto/wfa/measurement/api/v2alpha/measurement.proto line 238 at r3 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

I'd personally be fine with a DynamicClippedDeterministicCount methodology

Don't have a strong opinion on this but I notice the Population type also uses DeterministicCount methodology. Does DeterministicCount mean the same for Population and Impression?

https://github.com/world-federation-of-advertisers/cross-media-measurement-api/blob/main/src/main/proto/wfa/measurement/api/v2alpha/measurement.proto#L285

@stevenwarejones would like to follow up on the changes to make. Seems we still have 2 options here:

  1. (Current PR state)Add a filed dynamic_clipping_maximum_frequency_per_user into existing DeterministicCount methodology. If this field is specified, it means EDP calculates and uses the dynamic clipping frequency cap for impression measurement. If not, a fixed frequency cap is used by EDP for impression measurement which is the maximum_frequency_per_user in measurement_spec. But I am not sure if this way will confuse other measurement type which is also using DeterministicCount like POPULATION type?
  2. As suggested by Sanjay, add a new DynamicClippedDeterministicCount methodology which is more clear and specific. It can be justified that maximum frequency does affect the counting so it’s not really conflating concepts?

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.

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @chenweiw, @jiayu-google, @Marco-Premier, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Actually right now for impression, Measurement.Result only has the noised impression value instead of the capped frequency histogram so EDP doesn't return the actual cap being used

Ah sorry, I was mixing up R/F and Impression.

Done.

Copy link
Contributor

@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 4 files at r4, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @chenweiw, @jiayu-google, @Marco-Premier, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Done.

Does it always have to ben an optimized value? What if an MC specifies a clipped value higher than what an EDP supports? Should the EDP reject the measurement, or instead just fulfill it with the value it supports? If the latter, than I think calling this custom... better captures multiple motivations for populating this field.

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.

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

Does it always have to ben an optimized value? What if an MC specifies a clipped value higher than what an EDP supports? Should the EDP reject the measurement, or instead just fulfill it with the value it supports? If the latter, than I think calling this custom... better captures multiple motivations for populating this field.

This PR is to add the support of dynamic clipping impression based on the dynamic clipping library which will come with the optimized frequency cap value(good impression accuracy and dp guarantee under ACDP and Gaussian noise). I don’t know if right now we are supporting EDP to choose a custom frequency cap value that it supports? Or it sounds like the CustomDirectMethodology we already have? cc @kungfucraig @jiayu-google

Copy link
Contributor

@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 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

This PR is to add the support of dynamic clipping impression based on the dynamic clipping library which will come with the optimized frequency cap value(good impression accuracy and dp guarantee under ACDP and Gaussian noise). I don’t know if right now we are supporting EDP to choose a custom frequency cap value that it supports? Or it sounds like the CustomDirectMethodology we already have? cc @kungfucraig @jiayu-google

@kungfucraig @jiayu-google - I'm not sure we need a separate field for optimized vs custom. I'd prefer to just call this custom. Is the math on the reporting server side going to be any different in each case?

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.

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, and @riemanli)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

I'm not sure we need a separate field for optimized vs custom

We do if we explicitly don't want to support the "custom" case. My understanding is that we want only two options at the moment: using dynamic clipping as described by the library or use the static value provided in the MeasurmentSpec. @kungfucraig can correct me if my understanding of that is incorrect.

Copy link
Contributor

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I'm not sure we need a separate field for optimized vs custom

We do if we explicitly don't want to support the "custom" case. My understanding is that we want only two options at the moment: using dynamic clipping as described by the library or use the static value provided in the MeasurmentSpec. @kungfucraig can correct me if my understanding of that is incorrect.

the alternative is that an EDP will need to reject the requisition.

Copy link
Contributor

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, @riemanli, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

the alternative is that an EDP will need to reject the requisition.

also - optimized is quite vague - optimized for what the legal team says is the right value? or optimized based on a certain halo algorithm?

@iverson52000 iverson52000 force-pushed the alberthsuu-dynamicclip-impression branch from 0f6ae28 to 53ec27a Compare February 16, 2024 17:50
@iverson52000 iverson52000 changed the title Add Dynamic Clipping methodology for impression measurement Add custom_maximum_frequency_per_user field into DeterministicCount Feb 16, 2024
@iverson52000 iverson52000 changed the title Add custom_maximum_frequency_per_user field into DeterministicCount Add custom_maximum_frequency_per_user field into DeterministicCount methodology Feb 16, 2024
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.

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, @riemanli, @SanjayVas, and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

also - optimized is quite vague - optimized for what the legal team says is the right value? or optimized based on a certain halo algorithm?

Hi @stevenwarejones . Thanks for all the constructive feedback! After some internal discussion, we agree that the changes here should be a custom behavior for EDP instead of enforcing EDP to follow a specific algorithm. Update the field name to custom_maximum_frequency_per_user and the PR tile and description to reflect that. Please review. Although a custom cap might have some other implications in terms of the variance/bias calculation which requires more analysis by the data scientists.

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.

Reviewed 2 of 4 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chenweiw, @iverson52000, @jiayu-google, @kungfucraig, @Marco-Premier, and @riemanli)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 97 at r6 (raw file):

    // Maximum frequency per user that would be included in this measurement.
    //
    // If the custom_maximum_frequency_per_user in the DeterministicCount

Need to update this to match the changes in methodology messages.

Code quote:

    // If the custom_maximum_frequency_per_user in the DeterministicCount

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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, and @riemanli)


src/main/proto/wfa/measurement/api/v2alpha/direct_computation.proto line 97 at r4 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Hi @stevenwarejones . Thanks for all the constructive feedback! After some internal discussion, we agree that the changes here should be a custom behavior for EDP instead of enforcing EDP to follow a specific algorithm. Update the field name to custom_maximum_frequency_per_user and the PR tile and description to reflect that. Please review. Although a custom cap might have some other implications in terms of the variance/bias calculation which requires more analysis by the data scientists.

Hi @stevenwarejones. Did you have time to review the subsequent changes?

Copy link
Contributor

@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 r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chenweiw, @iverson52000, @jiayu-google, @kungfucraig, @Marco-Premier, and @riemanli)

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.

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


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 97 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Need to update this to match the changes in methodology messages.

Hi @SanjayVas. What should I update here?

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @chenweiw, @jiayu-google, @kungfucraig, @Marco-Premier, and @riemanli)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 97 at r6 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Hi @SanjayVas. What should I update here?

Looks like you've done the update. It was previously still referencing DynamicClipCount.

Copy link
Member

@kungfucraig kungfucraig 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 4 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @chenweiw, @jiayu-google, @Marco-Premier, and @riemanli)

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.

:lgtm:

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jiayu-google, @Marco-Premier, and @riemanli)

@iverson52000 iverson52000 force-pushed the alberthsuu-dynamicclip-impression branch from 53ec27a to 0bd40d5 Compare March 4, 2024 17:43
@iverson52000 iverson52000 merged commit f201ade into main Mar 4, 2024
4 checks passed
@iverson52000 iverson52000 deleted the alberthsuu-dynamicclip-impression branch March 4, 2024 17:47
iverson52000 added a commit to world-federation-of-advertisers/cross-media-measurement that referenced this pull request Mar 4, 2024
ple13 pushed a commit to world-federation-of-advertisers/cross-media-measurement 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.

8 participants