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

Optional output of Stats.min and Stats.max on series #419

Closed
zyzhu opened this issue Aug 13, 2018 · 4 comments
Closed

Optional output of Stats.min and Stats.max on series #419

zyzhu opened this issue Aug 13, 2018 · 4 comments

Comments

@zyzhu
Copy link
Contributor

zyzhu commented Aug 13, 2018

Stats.min and Stats.max on Series returns optional values by calling trySeriesExtreme. It returns None when the series is empty.

But this is the only stats function in the whole Stats module that returns optional value. Functions such as Stats.mean returns NaN when series is empty. Applying Stats.min and Stats.max on frame will also output missing value if the column is empty, essentially NaN from user's perspective.

Shall we make Stats.min and Stats.max consistent with other functions to return NaN if series is empty? Then users can avoid confusion such as the following SO question https://stackoverflow.com/questions/51606395/deedle-f-find-the-max-rows-within-an-index-group/51611467#51611467

@tpetricek
Copy link
Member

Yes, I would be in favour of a breaking change and just returning float.

The only caveat is that returning NaN won't work if we want to make the function work on series of integers, but I think that even throwing an exception (if we have to) is still better than the current design with options...

@zyzhu
Copy link
Contributor Author

zyzhu commented Aug 13, 2018

Forgot about integers. I'm afraid this breaking change requires try catch if anybody has implementation with min/max on integer series already.

Another option is to rename the current min/max with tryMin/tryMax. A new implementation of min and max can output NaN or throw an exception for integers.

@tpetricek
Copy link
Member

Renaming to tryMin and tryMax sound like a good approach!

@zyzhu
Copy link
Contributor Author

zyzhu commented Aug 14, 2018

Closed as it's addressed in #422

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