-
Notifications
You must be signed in to change notification settings - Fork 2
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
Remove ShareShuffleSketchParams #210
Conversation
8153e61
to
e72bcd0
Compare
There was a problem hiding this 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 r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kungfucraig and @ple13)
There was a problem hiding this 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, @ple13, and @renjiezh)
a discussion (no related file):
I assume this is due to the fact that HMSS doesn't actually use a sketch? If so, please add that to the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this break the simulator and perhaps other code too?
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ple13 and @renjiezh)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would normally be a breaking change, but nobody is using HMSS yet. That is to say it would break EDPs that fulfill HMSS requisitions, which right now is only our test environment simulators.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ple13 and @renjiezh)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you sanjay to illustrate it.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ple13 and @SanjayVas)
a discussion (no related file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
I assume this is due to the fact that HMSS doesn't actually use a sketch? If so, please add that to the PR description.
PR description added.
There was a problem hiding this 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 @ple13 and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 215 at r1 (raw file):
// // For ReachAndFrequency Measurement, it is required to be greater than // (1 + `maximum_frequency`).
It's worth noting that the maximum frequency is the number of EDPs multiplied by the maximum reportable frequency.
There was a problem hiding this 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 r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 215 at r1 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
It's worth noting that the maximum frequency is the number of EDPs multiplied by the maximum reportable frequency.
+1. It's more precise to be written as 1 + maximum_combined_frequency
or 1 + maximum_frequency*#EDPs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)
There was a problem hiding this 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, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)
In HMSS protocol we use FrequencyVector as the counterpart of Sketch in LLv2. This PR is to correct the naming. Besides, the content of FrequencyVectorParams (which was SketchParams) is only the ring modulus. Thus remove it.