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

fix: Julia 1.9 does not have _maybetail in Base #25

Closed
wants to merge 2 commits into from

Conversation

johnnychen94
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #25 (9464283) into master (d18900d) will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master      #25   +/-   ##
=======================================
  Coverage   66.66%   66.66%           
=======================================
  Files           1        1           
  Lines          42       45    +3     
=======================================
+ Hits           28       30    +2     
- Misses         14       15    +1     
Impacted Files Coverage Δ
src/EndpointRanges.jl 66.66% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d18900d...9464283. Read the comment docs.

@tpapp
Copy link

tpapp commented Jul 13, 2022

Given that 1.6 has both begin and end, I am wondering if it would make sense to cap at 1.8 and gracefully retire this package.

@rashidrafeek
Copy link

As was mentioned in a previous discussion by @timholy , this package has usefulness beyond the functionality available in Base. Basically, it allows to pass relative ranges as arguments to functions which isn't currently possible with Base.

I have been using this for the same to achieve similar functionality to python slice where it is possible to pass them to a function. For example, sometime I would need some function to act only a subset of the elements where the subset is passed as the input:

function foo(vectorA, mrange::Union{AbstractRange,EndpointRange})
    # Do something with vectorA[mrange]
end

It would not always make sense to pass vectorA[mrange] to the function as mrange might be unknown. For example, a cli program which takes mrange as a command line parameter. And then calls foo on the input vector.

I agree that the need of the package is less in julia compared to python due to presence of begin and end in Base. But it would be great if the package continues to be supported for some cases like the one described above.

Also see this related discussion in discourse.

@Tokazama
Copy link
Member

Some of this functionality might be easier to copy over into ArrayInterface.jl with just a couple lines of code. The ArrayInterface.to_indices is faster and has more robust multidimensional support than what's in base, but is obviously isn't as well tested.

@N5N3
Copy link

N5N3 commented Jul 18, 2022

Base.to_indices's stability have been improved on master.
I suggest to overload Base._to_indices1 instead to avoid possible instability:

@static if VERSION >= v"1.9.0-DEV.827"
    Base._to_indices1(A, inds, I1::Union{Endpoint, EndpointRange}) = (_newindex(inds, I1),)
else
    @inline function Base.to_indices(A, inds, I::Tuple{Union{Endpoint, EndpointRange}, Vararg{Any}})
        (_newindex(inds, I[1]), to_indices(A, Base.safe_tail(inds), Base.tail(I))...)
    end
end
_newindex(::Tuple{}, I1) = newindex(Base.OneTo(1), I1) # This also fix `randn(2)[1:iend,1:iend]`
_newindex(inds::Tuple, I1) = newindex(inds[1], I1)

@Tokazama
Copy link
Member

Inference is just one of the issues Base.to_indices has. It's slow, needlessly allocates, and has trouble keeping track of complex multidimensional situations. Some of this can be fixed with better compiler tools, but some of it is just built into its design

@N5N3
Copy link

N5N3 commented Jul 23, 2022

In fact, the allocation and slowness should all come from the runtime dispatch (At least for the examples in ArrayInterface.to_indices's doc)
The instability come from eager widening during inference on indirect self-recursion. It blocks our inline pass and cause runtime dispatch.
The current improvement on master is limited to index defined in Base. Packages all overload Base.to_indices and thus the recursion is not direct again.
I don't think it's a good idea to tell their maintainers that you'd better overload some internal api to avoid possible instability.

A more serious problem may be Base.to_ Indices and Base.checkbounds do not use the same recursion logic. For example [I::CartesianIndex{N}] is equivalent to [Tuple(I)...] for to_indices.
But that not true for checkbounds.

@Tokazama
Copy link
Member

How is it telling someone to overload some internal api?

The current design in base is flawed the in multiple and that's not changing anytime soon. We have a good set of tools for addressing this in ArrayInterface so it might be helpful to consider using it

@N5N3
Copy link

N5N3 commented Jul 24, 2022

How is it telling someone to overload some internal api?

Well, just as I mentioned above. User should overload Base._to_indices1/_cutdim rather than Base.to_indices to avoid possible instability on master.
They are internal apis, and some package like EllipsisNotation.jl can't overload them under the current design.

We have a good set of tools for addressing this in ArrayInterface so it might be helpful to consider using it.

ArrayInterface is wonderful in many cases. Hope we can port some design into 2.0 Base.

@Tokazama
Copy link
Member

How is it telling someone to overload some internal api?

Well, just as I mentioned above. User should overload Base._to_indices1/_cutdim rather than Base.to_indices to avoid possible instability on master. They are internal apis, and some package like EllipsisNotation.jl can't overload them under the current design.

Sorry, I thought you were saying that I had suggested overloading internal methods.

ArrayInterface is wonderful in many cases. Hope we can port some design into 2.0 Base.

I've been casually trying to figure out how to move towards easily injecting some of this improvements into Base.to_indices without it being breaking. But it's a bit complicated to do without sacrificing some advances we've made or placing a lot of manual intervention on the user.

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.

5 participants