-
Notifications
You must be signed in to change notification settings - Fork 18
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
Create min/max functions that take bounds into account #141
Conversation
cfe1c7b
to
4f760f6
Compare
Codecov Report
@@ Coverage Diff @@
## master #141 +/- ##
==========================================
+ Coverage 74.40% 75.76% +1.36%
==========================================
Files 11 11
Lines 461 491 +30
==========================================
+ Hits 343 372 +29
- Misses 118 119 +1
Continue to review full report at Codecov.
|
736521c
to
3dc8a8b
Compare
Another thing I'm thinking about is the name "precision". Currently Julia already has a |
As a follow-up to this PR, it'd be nice to resolve #90 😊 |
2342100
to
6d019b4
Compare
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
6d019b4
to
3d60fbb
Compare
Thanks for sticking with this. Things are shaping up nicely now :) |
Co-Authored-By: Curtis Vogt <1675958+omus@users.noreply.github.com>
Co-Authored-By: Curtis Vogt <1675958+omus@users.noreply.github.com>
|
||
# If we get to this point, the min/max functions throw a DomainError | ||
# Since we want our tests to be predictable, we will not throw an error in this helper. | ||
end |
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.
As theses helpers pretty much just re-implement minimum/maximum there aren't that helpful with testing. I think we can proceed with this but something to keep in mind for future work.
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 agree, in hindsight it would've been best to separate the tests per edge case and avoid having to do this. I constricted myself by trying to stay consistent with the first/last testing scheme and the test_values
that were already in place.
src/docstrings.jl
Outdated
minimum(interval::AbstractInterval{T}; [increment]) -> T | ||
|
||
The minimum value in the interval contained within the interval. |
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.
minimum(interval::AbstractInterval{T}; [increment]) -> T | |
The minimum value in the interval contained within the interval. | |
minimum(interval::AbstractInterval{T}; [increment]) -> T | |
The minimum value contained within the `interval`. |
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.
Looks like this change was missed?
src/docstrings.jl
Outdated
maximum(interval::AbstractInterval{T}; increment::Any) -> T | ||
|
||
The maximum value in the interval contained within the interval. |
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.
maximum(interval::AbstractInterval{T}; increment::Any) -> T | |
The maximum value in the interval contained within the interval. | |
maximum(interval::AbstractInterval{T}; [increment]) -> T | |
The maximum value contained within the `interval`. |
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.
Looks like this change was missed?
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.
With these last changes we should be good.
* Add min/max functions with precision * Add tests for new min/max functions * Add default val for closed/unbounded intervals in min/max * Rename min/max functions to minimum/maximum * Rename percision kwarg in min/max to increment * Add integer specific min/max function * Add float specific min/max functions * Respect the increment variable in AbstractFloat min/max * Apply suggestions and add more tests * Add unbounded accessor tests * Add anchoredinterval support in min/max * Adjust accessor test structure * Handle various edge cases in min/max * Add docstrings for min/max * Update isfinite.jl to support TimeTypes * Apply suggested changes * Update docs to include bound-open Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
PR was meant to be squashed. Squashed commit is c37cbb7 |
fixes #137