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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add parameter to histogramQuantile #5386

Merged
merged 4 commits into from
Feb 23, 2023
Merged

feat: add parameter to histogramQuantile #5386

merged 4 commits into from
Feb 23, 2023

Conversation

wolffcm
Copy link

@wolffcm wolffcm commented Feb 22, 2023

Closes #5385.

This adds a parameter to histogramQuantile that will let the user decide upon an alternative behavior when bin counts are not monotonically increasing. Currently only "error" is supported in this PR, I just wanted to get a bit of feedback before I do more.

As described in the Flux stdlib doc comments, this adds a parameter called onNonmonotonic that accepts three values:

  • error (default)
  • force
  • drop

@sanderson Do you have any thoughts on this approach? My thinking is that we have a similar parameter for filter called onEmpty, so I'm kind of following that pattern.

Checklist

Dear Author 馃憢, the following checks should be completed (or explicitly dismissed) before merging.

  • 鉁忥笍 Write a PR description, regardless of triviality, to include the value of this PR
  • 馃敆 Reference related issues
  • 馃弮 Test cases are included to exercise the new code
  • 馃И If new packages are being introduced to stdlib, link to Working Group discussion notes and ensure it lands under experimental/
  • 馃摉 If language features are changing, ensure docs/Spec.md has been updated

Dear Reviewer(s) 馃憢, you are responsible (among others) for ensuring the completeness and quality of the above before approval.

@sanderson
Copy link
Contributor

@wolffcm This looks good to me. I notice force and drop as other potential options in the source. Are these just placeholders?

@wolffcm
Copy link
Author

wolffcm commented Feb 22, 2023

I notice force and drop as other potential options in the source. Are these just placeholders?

Yes, that's right. They are unused, but I just wanted to indicate that they would be potential values.

@sanderson
Copy link
Contributor

Yes, that's right. They are unused, but I just wanted to indicate that they would be potential values.

Sounds good. Just wanted to make sure.

@wolffcm wolffcm marked this pull request as ready for review February 22, 2023 23:35
@wolffcm wolffcm requested review from a team as code owners February 22, 2023 23:35
@wolffcm wolffcm requested review from sanderson and removed request for a team February 22, 2023 23:35
@wolffcm wolffcm merged commit 726bf06 into master Feb 23, 2023
@wolffcm wolffcm deleted the feat/histo branch February 23, 2023 16:12
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.

Add parameter to histogramQuantile to control non-monotonic behavior
3 participants