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.

Sign up for GitHub

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

Jump to bottom

Deprecate rol and ror in favor of circshift methods #23404

Merged
merged 2 commits into from
Aug 31, 2017
Merged

Deprecate rol and ror in favor of circshift methods #23404

merged 2 commits into from
Aug 31, 2017

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Aug 23, 2017 •
edited
Loading

There are two commits here. The first is a simple rename, deprecating circshift and circshift! in favor of rotate and rotate!, respectively. The second deprecates rol/rol! and ror/ror! in favor of specialized methods of rotate/rotate! circshift/circshift!.

Fixes #19923.

@ararslan ararslan added domain:collections Data structures holding multiple items, e.g. sets kind:deprecation This change introduces or involves a deprecation labels Aug 23, 2017
@stevengj
Copy link
Member

stevengj commented Aug 23, 2017 •
edited
Loading

rotate seems much less clear to me than circshift. I can more easily imagine rotate being used by packages for some kind of Euclidean rotation function, for example.

@KristofferC
Copy link
Sponsor Member

I can more easily imagine rotate being used by packages for some kind of Euclidean rotation function, for example.

https://github.com/KristofferC/Tensors.jl/blob/d5c8d97a71d981d01aaf160d94e0a3abf01a4060/src/math_ops.jl#L361

@musm
Copy link
Contributor

musm commented Aug 23, 2017 •
edited
Loading

I like circshift or some variation of that name, since, e.g., when textbooks discuss circulant matrices they typically refer to each row as being obtained by circularly shifting (or circularly translating) the previous row:

@ararslan
Copy link
Member Author

rotate seems much less clear to me than circshift

If people prefer circshift that's fine with me and I can drop the first commit, but personally I find rotate to be a clearer and more discoverable name.

@stevengj
Copy link
Member

Matlab uses circshift, NumPy uses roll, Fortran and Scilab use cshift, IDL uses shift.... I would strongly prefer circshift to rotate.

@ararslan
Copy link
Member Author

You're omitting APL, which calls this ⌽ and ⊖. Clearly those are the superior names. :trollface:

Sure, I can make it circshift.

@ararslan ararslan changed the title Rename circshift to rotate, deprecate rol and ror to rotate methods Deprecate rol and ror in favor of circshift methods Aug 23, 2017
@iamed2
Copy link
Contributor

iamed2 commented Aug 24, 2017

Could you include the word "rotate" in the help text so I can do apropos("rotate") to find the method? I can never remember the name.

@StefanKarpinski
Copy link
Sponsor Member

circshift and circshift! are fine as long as we make it consistent and have the word "rotate" in the docstrings.

@ararslan
Copy link
Member Author

Okay, added the word "rotate" to the docstrings.

@ararslan
Copy link
Member Author

The FreeBSD and macOS errors are unrelated.

@ararslan
Copy link
Member Author

Any feedback on this?

stevengj
stevengj reviewed Aug 30, 2017
View reviewed changes
base/bitarray.jl
```
"""
ror(B::BitVector, i::Integer) = ror!(similar(B), B, i)
circshift!(B::BitVector, i::Integer) = circshift!(B, B, i)
Copy link
Member

@stevengj stevengj Aug 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a single-array circshift! for non-bitvectors? I guess not, but it's a bit weird to have this only for one specialized array type (maybe re-open #16032).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've reopened #16032. Is the disconnect in methods blocking for this PR or can that be added later?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do that separately.

mbauman
mbauman reviewed Aug 30, 2017
View reviewed changes
base/abstractarraymath.jl Outdated
Circularly shift the data in an array. The second argument is a vector giving the amount to
shift in each dimension.
Circularly shift, i.e. rotate, the data in an array. The second argument is a vector giving
the amount to shift in each dimension.
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… or an integer to only shift in the first dimension.

@ararslan ararslan mentioned this pull request Aug 30, 2017
Open
@ararslan
Add NEWS entry
aa17eca 
[ci skip]
@ararslan ararslan merged commit c64a6cc into master Aug 31, 2017
@ararslan ararslan deleted the aa/rotate branch August 31, 2017 17:16
@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Aug 31, 2017
@cormullion cormullion mentioned this pull request Mar 18, 2018
Closed
@mbauman mbauman mentioned this pull request Feb 4, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman mbauman left review comments

@stevengj stevengj stevengj left review comments

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

@kshyatt kshyatt Awaiting requested review from kshyatt

Assignees

@ararslan ararslan

Labels
domain:collections Data structures holding multiple items, e.g. sets kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.
7 participants
@ararslan @stevengj @KristofferC @musm @iamed2 @StefanKarpinski @mbauman