-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 offset ranges when comparing ranges #54825
base: master
Are you sure you want to change the base?
Conversation
So it seems that there are no fundamental problems with this PR. The only thing that "breaks" is that some tests compare ranges when they only want to compare their values. To make this easier, I have defined My main question now is: Do you want to proceed in this direction at all? |
4beaea3
to
c89ff5b
Compare
I did a force push, and this made the tests (in the checks) disappear. Now one cannot see anymore that they were all passes. To find out if there are problems with other packages, I searched for all registered packages with repos on GitHub that use
In total this looks promising. However, I'm surprised how many packages don't compile or fail their own tests on master. (For my tests I combined this PR with #54891 to get master working on my machine.) |
I'm not qualified to answer this, sorry.
This effort wasn't required from you, FTR, 😅. The "needs pkgeval" label indicates that Julia's Nanosoldier infrastructure should be utilized to do what you did, but in an automated and systematic manner. At least that's what I meant when I added the label. Could you rebase the changes on current master again, then I'll schedule a Nanosoldier PkgEval? Sorry for the confusion, and for the late answer. EDIT: I'll try to fix the FillArrays bug before scheduling the PkgEval, to make the PkgEval more relevant.
Could you make PRs for the relevant packages, where it makes sense, given that you already have the changes on your Github? If you do this, please xref this PR (like |
This PR is prompted by the issue #54783. There I mentioned that indices are not taken into account when comparing ranges, while for
AbstractArray
in general they do matter. (I'm not the first to notice this.) I stumbled upon this while working on the small package OffsetRanges.jl, but I think this is relevant in general.The purpose of the PR is to see if one can take indices into account when comparing ranges. I've run the
ranges
tests (with offset ranges added) and they worked fine. UPDATE: all tests passIf one wants to address this issue, then the first question would be where offset ranges would live in the type system. One approach would be to allow existing range types to have elements that are not 1-based. This is currently done with
Base.IdentityUnitRange
, which is a subtype ofAbstractUnitRange{Int}
. This is also done with this PR, and it is the approach currently taken by OffsetArrays.jl. A different approach would be to copy the whole type tree. For example, there could beGeneralOrdinalRange
that containsOrdinalRange
(1-based) andOffsetOrdinalRange
(variable first index). This way all existing methods for 1-based ranges would continue to work.As said above, this PR is only meant to see if including indices when comparing ranges does work, and maybe to get further feedback. In one commit I delete the method
==(r::AbstractRange, s::AbstractRange)
because I think it's redundant. It checks if the two ranges have equal elements, what is exactly what the general method forAbstractArray
does (modulo checking that the axes are equal).