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

Stop warning about NaNs #238

Merged
merged 1 commit into from
Aug 20, 2018
Merged

Conversation

aaron-mcdaid-zalando
Copy link
Collaborator

Stop warning about NaNs. They are perfectly legitimate datapoints, when using derived KPIs

Longer term, perhaps there is an appropriate place to print the numbers of NaNs in the input data, in a nicely-tabulated piece of text. But the current warning is very spammy

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 92.216% when pulling 7f71e5d on drop.the.NaN.warnings into 59ccf56 on master.

@gbordyugov
Copy link
Contributor

@aaron-mcdaid-zalando could you pls elaborate on that? What happens if all of the data points are NaNs? Do we accept NaNs as valid values for means/std/p-values?

@aaron-mcdaid-zalando
Copy link
Collaborator Author

@gbordyugov,

Currently, for means/std/p-values/sample-sizes, the NaNs are discarded. So the NaN values aren't really an issue (except that expan currently includes the NaNs when attempting to decide if there is a sufficient number of datapoints).

So, we should forget about the NaNs, and instead count the number of non-NaNs.

Longer term, a lot of this needs to be redone entirely though :-)

@aaron-mcdaid-zalando aaron-mcdaid-zalando merged commit ef6a46a into master Aug 20, 2018
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.

4 participants