-
Notifications
You must be signed in to change notification settings - Fork 71
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
Allow pointwise equality #604
Allow pointwise equality #604
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #604 +/- ##
==========================================
- Coverage 84.72% 84.30% -0.42%
==========================================
Files 23 23
Lines 2049 2077 +28
==========================================
+ Hits 1736 1751 +15
- Misses 313 326 +13 ☔ View full report in Codecov by Sentry. |
That's painful. I guess however that this is mostly vocabulary. I assume And I think our version of |
Good point. I added a docstring with a "note admonition" to highlight the fact that we handle equality between two intervals differently. |
I gained a better feeling of all the changes we made in v0.22 after updating a bunch of code I have.
One recurrent and painful item was dealing with
iszero
andisone
, which both @lbenet and @timholy raised concerns about. For instance, in PR #586, the removal ofiszero
broke ForwardDiff compatibility. At the time, I felt that this was not so bad since there was a potential hook.Yet, I also realised that this also breaks SparseArrays which is presumably much more difficult to fix (it does not seem realistic to ask the entire ecosystem to provide us for a hook...). E.g
Additionally, because
iszero
andisone
are pointwise by nature, they can not yield ambiguous answers: the interval is, or not, a zero/one singleton.This is in stark contrast with other boolean operations which are rightfully disallowed; to illustrate:
<
also implies some ordering, which may lead to ambiguous answersisfinite
may be ambiguous depending on the flavour (but could be brought back by using @Kolaru's flavour mechanism...)isnan
conflicts withisnai
Therefore, I am proposing to bring back
iszero
andisone
. Moreover, our functionisthin(::Interval, ::Number)
is by definition just a generalisation ofisthinzero
andisthinone
for anyNumber
. Thus,==(::Interval, ::Number)
is safe ifiszero
andisone
are considered safe.Lastly, I think that
isinteger
is also safe since integers are necessarily finite.Note
I think this is not a breaking PR as
isthinzero
/isthinone
/isthin
/isthininteger
would still be defined and exported.However, if we do end up liking these changes by the end of 0.22; I think we ought to remove
isthinzero
/isthinone
/isthin
/isthininteger
for the 1.0 release as we should not have distinct functions do the same operation.