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

weightedAvg throws "index out of bounds" exception #94

Closed
nmattia opened this issue Jun 24, 2016 · 3 comments
Closed

weightedAvg throws "index out of bounds" exception #94

nmattia opened this issue Jun 24, 2016 · 3 comments

Comments

@nmattia
Copy link
Contributor

nmattia commented Jun 24, 2016

When weightedAvg is called on an empty vector the following exception is thrown:

> import Statistics.Quantile as Q
> import Data.Vector as V
> Q.weightedAvg $ V.fromList []
 *** Exception: ./Data/Vector/Generic.hs:235 ((!)): index out of bounds (-1,0)

Unfortunately most input errors seem to be handling with exceptions. To be nice to the user, the error could be made more explicit:

> import Statistics.Quantile as Q
> import Data.Vector as V
> Q.weightedAvg $ V.fromList []
 *** Exception: Statistics.Quantile.weightedAvg: Sample is empty

mean and welfordMean return NaN and 0 resp. on empty input. Would that be a solution? Either way, it would be good to document which parameters can be used such that the user can be extra careful.

@Shimuuar
Copy link
Collaborator

I agree library function should not throw out of bounds access errors. They aren't more helpful than dreaded Prelude.head: empty list.

Situation with mean is little trickier. Mean is not defined for empty sample so we have to either throw error for empty sample or wrap result in Maybe or add define result for empty sample. Both mean and welfordMean are defined in terms of left fold so there's base case for empty sample and it happens to be NaN and 0 respectively.

@nmattia
Copy link
Contributor Author

nmattia commented Jun 26, 2016

Same applies for quantiles, they're not defined for an empty input. I guess the ideal solution would indeed be to wrap everything in Maybes, or provide an alternative package/module like safe to avoid breaking compatibility. Though I believe that a meaningful error message is already an improvement over the current behavior. Feel free to close this if it's satisfactory.

@Shimuuar
Copy link
Collaborator

I'm closing this in favor of #100. Problem with weightedAvg is fixed and other issue is more general

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

No branches or pull requests

2 participants