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

Show information gain in header validator output #1157

Merged
merged 6 commits into from
Feb 2, 2024
Merged

Conversation

apasel422
Copy link
Collaborator

@apasel422 apasel422 commented Feb 1, 2024

We also change the command-line output to resemble that of the event-level reports by using randomized trigger rate instead of flip probability and decimals instead of percentages.

@apasel422 apasel422 marked this pull request as ready for review February 1, 2024 21:58
@apasel422 apasel422 requested a review from csharrison February 1, 2024 21:58
@csharrison
Copy link
Collaborator

Open questions:

  1. Should we retain the flip percent terminology from the command-line tool?
  2. Should we show the value as a percentage or decimal in [0, 1]?

For both of these I would argue we should make the output resemble that of the event-level reports, and go with randomized_trigger_rate and [0, 1]. But regardless, we should probably be consistent in the command-line output and the header validator output.

This all seems fine. We can update the command line output to align.

@csharrison
Copy link
Collaborator

Otherwise the PR LG

@apasel422
Copy link
Collaborator Author

Open questions:

  1. Should we retain the flip percent terminology from the command-line tool?
  2. Should we show the value as a percentage or decimal in [0, 1]?

For both of these I would argue we should make the output resemble that of the event-level reports, and go with randomized_trigger_rate and [0, 1]. But regardless, we should probably be consistent in the command-line output and the header validator output.

This all seems fine. We can update the command line output to align.

Ack. I think we'll need to use at least toFixed(5) for the decimal change in order for the default event rate to show up as non-zero. Do you think we should go beyond even 5?

@csharrison
Copy link
Collaborator

Can we match the impl which limits to 7 digits of precision after the decimal point?

@apasel422
Copy link
Collaborator Author

Can we match the impl which limits to 7 digits of precision after the decimal point?

Done.

@apasel422 apasel422 merged commit 7a4487f into WICG:main Feb 2, 2024
2 checks passed
@apasel422 apasel422 deleted the show branch February 2, 2024 13:18
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.

2 participants