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

Equality of empty ranges #29421

Closed
sostock opened this issue Sep 28, 2018 · 19 comments · Fixed by #32348
Closed

Equality of empty ranges #29421

sostock opened this issue Sep 28, 2018 · 19 comments · Fixed by #32348
Labels
arrays [a, r, r, a, y, s] minor change Marginal behavior change acceptable for a minor release needs decision A decision on this change is needed

Comments

@sostock
Copy link
Contributor

sostock commented Sep 28, 2018

In order to be equal, ranges need to have the same first element, last element, and step, even if they are empty. Beside being unintuitive, this also breaks the transitivity of ==:

julia> 2:1 == 1:0
false

julia> 2:1 == Int[] == 1:0
true
@nalimilan
Copy link
Member

There's likely a historical reason to this: before #16401, arrays and ranges could never be equal, so there was no transitivity issue. We should definitely change that IMHO.

@mschauer
Copy link
Contributor

This is a reasonable change, but it would be at odds with the searchsorted interface, for which the elements of an empty range have a meaning and shouldn't compare equal

julia> searchsorted([1.0,2.0,3.0],2.5)
3:2

julia> searchsorted([1.0,2.0,3.0],2.5) == 2:1
false

@nalimilan
Copy link
Member

Yes, that's related to #22354. But I don't think that's a problem here. The result of searchsorted gives you additional information via the start and end values of an empty range, but it's also correct to ignore that information and just say that the value wasn't found.

@nalimilan nalimilan added the triage This should be discussed on a triage call label Oct 1, 2018
@nalimilan
Copy link
Member

Marking for triage since that would be technically breaking.

@nalimilan nalimilan added needs decision A decision on this change is needed triage This should be discussed on a triage call and removed triage This should be discussed on a triage call labels Oct 1, 2018
@JeffBezanson JeffBezanson added the arrays [a, r, r, a, y, s] label Oct 1, 2018
@mauro3
Copy link
Contributor

mauro3 commented Oct 1, 2018

But isn't == supposed to be transitive? If so, this would be a bug.

@StefanKarpinski
Copy link
Member

Yes, we can definitely make that case but we still have to consider the consequences. Definitely need discussion in any case.

@StefanKarpinski
Copy link
Member

Triage generally feels that these should be made == and isequal. We make things value-equal all the time that have details which differ and this seems no different. Transitivity of equality is more important. Would need a PkgEval run to see what if anything breaks.

@JeffBezanson
Copy link
Member

I generally agree with that but searchsorted does kind of throw a monkey wrench into it.

@StefanKarpinski
Copy link
Member

I'm a bit hazy on when you would compare the result of searchsorted like that. Can someone thing of realistic example of that kind of usage?

@mschauer
Copy link
Contributor

mschauer commented Nov 3, 2018

In interpolation at i = searchsorted(v, x), checking for i == 1:0 or i == n+1:n to special case boundaries seems natural.

@StefanKarpinski
Copy link
Member

Right, but when would it be a problem that different empty ranges returned by searchsorted are considered to be equal?

@StefanKarpinski
Copy link
Member

It seems like the only way this could be a problem would be if you use searchsorted on two different arrays and want to see if they would insert values at the same location. That seems like a fairly contrived situation. We'll have to run PkgEval on this change and see if it breaks things in practice. If it does we'll have to live with this violation of transitivity until 2.0; if it doesn't we can do it in 1.1.

@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Nov 15, 2018
@StefanKarpinski StefanKarpinski added this to the 1.1 milestone Nov 15, 2018
@StefanKarpinski StefanKarpinski added help wanted Indicates that a maintainer wants help on an issue or pull request minor change Marginal behavior change acceptable for a minor release labels Nov 15, 2018
@StefanKarpinski StefanKarpinski modified the milestones: 1.1, 1.2 Dec 6, 2018
@StefanKarpinski
Copy link
Member

This is a fairly significant change, so it would be good if we got it done sooner rather than later but it's a bit too late for this to happen in 1.1 at this point.

@KristofferC KristofferC modified the milestones: 1.2, 1.x Apr 17, 2019
@rfourquet rfourquet removed the help wanted Indicates that a maintainer wants help on an issue or pull request label May 4, 2020
@ti-s
Copy link

ti-s commented May 29, 2020

Should this be changed as well?

julia> 3:3 == 3:2:4
false

julia> 3:3 == [3] == 3:2:4
true

@sostock
Copy link
Contributor Author

sostock commented May 29, 2020

Should this be changed as well?

I think so. I’m kinda surprised that I never noticed that.

@sostock
Copy link
Contributor Author

sostock commented May 29, 2020

I would even say that this should be changed and backported to 1.5, since the behavior in 1.5 is now very confusing. I would consider it a bug.

@rfourquet
Copy link
Member

@sostock I agree, could you open an issue so this can be discussed (or a (possibly draft) PR if you feel like) and not forgotten?

@sostock
Copy link
Contributor Author

sostock commented May 29, 2020

Yeah, I will open a PR.

@sostock
Copy link
Contributor Author

sostock commented May 29, 2020

I have opened a PR: #36074

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] minor change Marginal behavior change acceptable for a minor release needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants