-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 support for filters to T-Test aggregation #54980
Add support for filters to T-Test aggregation #54980
Conversation
Adds support for filters to T-Test aggregation. The filters can be used to select populations based on some criteria and use values from the same or different fields. Closes elastic#53692
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
@elasticmachine update branch |
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.
Left a question about validation, but otherwise looks good. Unpaired fits in nicely, I like how this turned out :)
return new PairedTTestAggregator(name, numericMultiVS, tails, format, searchContext, parent, metadata); | ||
case HOMOSCEDASTIC: | ||
return new UnpairedTTestAggregator(name, numericMultiVS, tails, true, format, searchContext, parent, metadata); | ||
return new UnpairedTTestAggregator(name, numericMultiVS, tails, true, this::getWeights, format, searchContext, parent, |
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.
Should we validate/throw an exception/something if people specify an unpaired type, but don't provide filters? Not sure where to put that validation logic but it seems we should disallow folks from basically running an unpaired test on two match-all's?
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.
I was thinking about support for something like this:
{"before": [1,2,3], "after":[6,7,8,9,10]}
It currently works for unpaired, fails on paired and doesn't require filters.
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.
LGTM pending what we discussed offline (failing if field is same between A/B and there is no filter) 👍
@elasticmachine update branch |
Adds support for filters to T-Test aggregation. The filters can be used to select populations based on some criteria and use values from the same or different fields. Closes elastic#53692
Adds support for filters to T-Test aggregation. The filters can be used to
select populations based on some criteria and use values from the same or
different fields.
Closes #53692