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 PrivacyQueryMapper for ACDP composition #1163

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

iverson52000
Copy link
Contributor

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

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 r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @iverson52000, @RibeiroAna, and @uakyol)


src/main/kotlin/org/wfanet/measurement/eventdataprovider/privacybudgetmanagement/api/v2alpha/PrivacyQueryMapper.kt line 29 at r1 (raw file):

import org.wfanet.measurement.eventdataprovider.privacybudgetmanagement.Reference

private const val SENSITIVITY = 1.0

this should initialized with the object/class imo

@iverson52000 iverson52000 force-pushed the alberthsuu-acdp-pbm-querymapper branch from 13d020e to 70da7ef Compare August 10, 2023 17:57
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 3 files reviewed, 1 unresolved discussion (waiting on @RibeiroAna, @stevenwarejones, and @uakyol)


src/main/kotlin/org/wfanet/measurement/eventdataprovider/privacybudgetmanagement/api/v2alpha/PrivacyQueryMapper.kt line 29 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

this should initialized with the object/class imo

Done. Make sense thanks

@uakyol
Copy link
Contributor

uakyol commented Aug 10, 2023

src/main/kotlin/org/wfanet/measurement/eventdataprovider/privacybudgetmanagement/api/v2alpha/PrivacyQueryMapper.kt line 29 at r1 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Done. Make sense thanks

Do we ever want to initialize the PBM with a different sensitivity?

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 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @RibeiroAna and @uakyol)

Copy link
Contributor

@uakyol uakyol 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 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RibeiroAna and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/eventdataprovider/privacybudgetmanagement/api/v2alpha/PrivacyQueryMapper.kt line 29 at r1 (raw file):

Previously, uakyol wrote…

Do we ever want to initialize the PBM with a different sensitivity?

Synced offline, seems we would need to do this.

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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RibeiroAna and @uakyol)


src/main/kotlin/org/wfanet/measurement/eventdataprovider/privacybudgetmanagement/api/v2alpha/PrivacyQueryMapper.kt line 29 at r1 (raw file):

Previously, uakyol wrote…

Synced offline, seems we would need to do this.

change this to a class with the sensitivity passed in to the constructor then?

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 @RibeiroAna and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/eventdataprovider/privacybudgetmanagement/api/v2alpha/PrivacyQueryMapper.kt line 29 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

change this to a class with the sensitivity passed in to the constructor then?

The SENSITIVITY is subject to change in the future in some ways. From Pasin, the SENSITIVITY may be different from 1.0 for impression and duration. It may be fixed constant, or an output from another function depends on the design of other ACDP PBM add-on features. I suggest we leave PrivacyQueryMapper as it is for now and update it later when needed.

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.

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

@iverson52000 iverson52000 force-pushed the alberthsuu-acdp-pbm-querymapper branch from 70da7ef to e0037b4 Compare August 15, 2023 16:22
@iverson52000 iverson52000 merged commit 2d0bd06 into main Aug 15, 2023
@iverson52000 iverson52000 deleted the alberthsuu-acdp-pbm-querymapper branch August 15, 2023 16:54
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