Added MaxContributions to privacy-on-beam/pbeam/count.go#337
Added MaxContributions to privacy-on-beam/pbeam/count.go#337kutay25 wants to merge 2 commits intogoogle:mainfrom
Conversation
miracvbasaran
left a comment
miracvbasaran
left a comment
There was a problem hiding this comment.
LG overall, thanks a lot for working on this!
…ciated codepath at go/dpagg
|
|
||
| if maxContributionsSet { | ||
| // MaxContributions configuration must be used | ||
| if maxValue != 0 || maxPartitionsContributed != 0 { |
There was a problem hiding this comment.
We should use if maxValueSet || maxPartitionsContributedSet here, or get rid of {maxContributionsSet, maxValueSet, maxPartitionsContributedSet} and just check for positive in the if statements
| return fmt.Errorf("when MaxContributions is set, MaxValue and MaxPartitionsContributed must be 0") | ||
| } | ||
| return nil | ||
| } else { |
There was a problem hiding this comment.
No need for else, we return in the previous segment
| PreThreshold int64 | ||
| // MaxPartitionsContributed is the number of distinct partitions a single | ||
| // privacy unit can contribute to. Required. | ||
| // privacy unit can contribute to. For PreAggSelectPartition, MaxPartitionsContributed |
There was a problem hiding this comment.
nit: "For PreAggSelectionPartition, setting MaxPartitionsContributed is functionally the same as setting MaxContributions, but are kept separate for consistency with other aggregations."
(also the same below)
| // This does not hold for privacy-ID count aggregations, where l0=MaxContributions and lInf=1. | ||
| lInf = opt.MaxContributions | ||
| // When using MaxContributions, no per-partition contribution bounding is performed. | ||
| // upper is set to math.MaxInt64 to avoid clamping any contributions. |
There was a problem hiding this comment.
nit: upper is set to a default of match.MaxInt64 because it is being used in the Add function below, making it a no-op.
| if maxContributions <= 0 && maxPartitionsContributed <= 0 { | ||
| return 0, fmt.Errorf("Exactly one of MaxContributions and MaxPartitionsContributed must be set") |
There was a problem hiding this comment.
We should be able to use checks package here, no?
| } | ||
| if maxContributions > 0 { | ||
| return 1, nil | ||
| } else { |
There was a problem hiding this comment.
Again, no need for else after returning
| return fmt.Errorf("MaxContributions and MaxPartitionsContributed are both set, exactly one should be set.") | ||
| } | ||
| if maxContributions <= 0 && maxPartitionsContributed <= 0 { | ||
| return fmt.Errorf("MaxContributions and MaxPartitionsContributed are both unset, exactly one should be set.") |
There was a problem hiding this comment.
super-nit: no punctuation should be used at the end of error strings in golang
2 participants
Added MaxContributions to privacy-on-beam/pbeam/count.go and the associated codepath at go/dpagg.
The proposal can be found in this document.