-
Notifications
You must be signed in to change notification settings - Fork 13
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 more study validation from processed_study.cc. #1183
Conversation
); | ||
} | ||
|
||
// Validate total probability. | ||
if (totalProbability !== 100) { |
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.
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.
We can assume totalProbability == 100
for Griffin, but not for Finch, so the code will be non-reusable.
Not sure that it's a real issue, because we don't validate Finch right now.
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.
We can assume
totalProbability == 100
for Griffin
yes, that's what we did before and I assume we will continue doing. It's just easier to view this as 0-100 percent value.
// Validate features. | ||
if (experiment.feature_association !== undefined) { | ||
if ( | ||
experiment.feature_association.enable_feature !== undefined && |
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.
Can we use experiment?.feature_association
to remove if (experiment.feature_association !== undefined)
?
Also, !==undefined
can't be get rid off if it's an object.
experiment.feature_association.enable_feature !== undefined && | ||
experiment.feature_association.enable_feature.length > 0 | ||
) { | ||
for (const feature of experiment.feature_association.enable_feature) { |
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.
WDYT about making let features_to_check: string[] = []
and extending it.
Then just a one for-loop to check them all.
// Valiate params. | ||
const paramNames = new Set<string>(); | ||
if (experiment.param.length > 0) { | ||
for (const param of experiment.param) { |
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.
nit: for (const param of experiment.param || [])
2320048
to
d94d251
Compare
d94d251
to
6174eb6
Compare
Implement additional validation that exists in processed_study.cc.
Important ones:
Other was added just to have the full coverage (forcing_flag, google_web_experiment_id).