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

Deprecate circshift() and make it lazy #25003

Open
nalimilan opened this issue Dec 9, 2017 · 14 comments
Open

Deprecate circshift() and make it lazy #25003

nalimilan opened this issue Dec 9, 2017 · 14 comments

Comments

@nalimilan
Copy link
Member

This came up in a Slack discussion about adding lag and lead functions, which are similar to circshift. In many cases, allocating a vector to store the result of circshift is a waste, as a lazy view would be sufficient and as efficient. Looking at uses of circshift on GitHub shows several examples where the result is discarded immediately.

It would make sense to have Iterators.circshift, returning a custom AbstractArray object which would redirect getindex calls to the parent after shifting the index. circshift!(dest, src, shift) would be replaced with copy!(dest, Iterators.circshift(src, shift)).

@StefanKarpinski
Copy link
Member

Do we need it in Base at all? Seems like there are no uses, only definitions, which suggest to me that it would make a good stdlib package.

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Dec 9, 2017
@nalimilan
Copy link
Member Author

Probably, but what package? Something were we'd also put lag and lead?

@StefanKarpinski
Copy link
Member

Initially, it could just be stdlib/CircularShifts or something. Could later be rolled into a more general package for utility views into arrays.

@nalimilan
Copy link
Member Author

We could try to find a slightly more general name from the start to avoid renaming it if possible. lag and lead are not "circular" for example. LazyArrays? ArrayTools?

@tknopp
Copy link
Contributor

tknopp commented Dec 10, 2017

For me this seem to belong to a module that groups: Array slicing, Array reshaping, Array transpose. Its not super necessary that circshift sits in base. But discoverability is IMHO pretty important.

@nalimilan
Copy link
Member Author

Slicing, reshaping and transpose are really basic features, I'm not sure we want to move them to stdlib...

@bramtayl
Copy link
Contributor

If we're talking about slicing JuliennedArrays.jl does slicing much faster than mapslices in Base and also is much more flexible.

@StefanKarpinski
Copy link
Member

JuliennedArrays is the best-named package.

@bramtayl
Copy link
Contributor

be still my heart

@JeffBezanson
Copy link
Member

We can probably punt on this for 1.0.

@nalimilan
Copy link
Member Author

That's clearly not a big issue, but if we want to deprecate the functions something will have to be done before 1.0 (either a deprecation or moving them to stdlib).

@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Dec 14, 2017
@StefanKarpinski
Copy link
Member

This can be a vestigial API at some point in the future when a lovely, complete lazy package exists.

@piever
Copy link
Contributor

piever commented Feb 16, 2018

Implemented in the ShiftedArrays package, together with lag and lead. The two new AbstractArray types are ShiftedArray (meaning: you get missing if you are out of bounds) for lag and lead and CircShiftedArray (meaning, you restart from the other extreme) for circshift.

@StefanKarpinski
Copy link
Member

I guess the question now is what do we want the relationship between this Base function and the ShiftedArrays package to be? Should we consider deprecating it entirely and pointing people at the package instead? Or should we retain this as a way of doing eager array shifting?

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

No branches or pull requests

6 participants