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 flipdim to reverse with a dims keyword argument #26369

Merged
merged 1 commit into from
Mar 10, 2018
Merged

deprecate flipdim to reverse with a dims keyword argument #26369

merged 1 commit into from
Mar 10, 2018

Conversation

JeffBezanson
Copy link
Member

I think this is another case that fits well with the dims keyword argument change. The docstring for flipdim describes the operation as "reverse", often a telltale sign.

@JeffBezanson JeffBezanson added arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation labels Mar 8, 2018
@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Mar 8, 2018
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Mar 8, 2018
@JeffBezanson
Copy link
Member Author

Triage accepts.

@StefanKarpinski
Copy link
Member

Triage approves. Supporting reversing multiple dimensions is left as future work.

ararslan
ararslan reviewed Mar 8, 2018
View reviewed changes
base/abstractarraymath.jl Outdated
2×2 Array{Int64,2}:
2 1
4 3
```
"""
function flipdim(A::AbstractArray, d::Integer)
function reverse(A::AbstractArray; dims::Integer)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this overwrite the reverse method for arrays with one with a required keyword argument?

Copy link
Member

Choose a reason for hiding this comment

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

That one is only defined for AbstractVector

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, thanks.

@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Mar 8, 2018
ararslan
ararslan reviewed Mar 8, 2018
View reviewed changes
base/abstractarraymath.jl Outdated
2×2 Array{Int64,2}:
2 1
4 3
```
"""
function flipdim(A::AbstractArray, d::Integer)
function reverse(A::AbstractArray; dims::Integer)
nd = ndims(A)
1 ≤ d ≤ nd || throw(ArgumentError("dimension $d is not 1 ≤ $d ≤ $nd"))
Copy link
Member

@ararslan ararslan Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This should be updated to reflect the change from the argument being called d to being called dims. (That's what's causing the current CI failures.)

Copy link
Member Author

Choose a reason for hiding this comment

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

ooops

@stevengj
Copy link
Member

stevengj commented Mar 9, 2018

Should the old reverse(::AbstractVector) functions now take a dims=1 keyword argument?

Otherwise it seems like it may be a problem for generic code calling reverse(A, dims=1) where A is sometimes a matrix and sometimes a vector, for example, since in the latter case it dispatches to a method that doesn't take keywords?

@JeffBezanson
Copy link
Member Author

Yes, you're right.

@JeffBezanson
Copy link
Member Author

Ah, actually it happens to work currently due to #9498 --- passing a keyword argument causes us to favor a matching method that accepts them.

@stevengj
Copy link
Member

stevengj commented Mar 9, 2018 •
edited
Loading

Would be good to at least have a test for reverse([1,2,3], dim=1) == [3,2,1] if we don't have one already, to make sure that this doesn't get broken if the dispatch rules change.

@stevengj
Copy link
Member

stevengj commented Mar 9, 2018

Would also be good to have an in-place flipdim!, i.e. a reverse! method that accepts the dim keyword, but that is a separate issue.

@JeffBezanson JeffBezanson merged commit 8c2065c into master Mar 10, 2018
@JeffBezanson JeffBezanson deleted the jb/reversedim branch March 10, 2018 22:01
@stevengj stevengj mentioned this pull request May 12, 2018
Closed
@stevengj stevengj mentioned this pull request Jun 8, 2018
Closed
johnnychen94 added a commit to Evizero/Augmentor.jl that referenced this pull request Jan 29, 2020
@johnnychen94
update codebase to 1.0 and 1.x (continue #27) (#31)
7a30087 
* add Project.toml

* update test for cache and minor fix

* update test for rotation and minor fix

* update flipdm to reverse

JuliaLang/julia#26369

* update test for crop and minor fix

* update test for resize

* update test for scale and minor fix

* update test for zoom and minor fix

* WIP: update test for distortion and minor fix

* update test for either and minor fix

* fix pipeline test

* update summary reference for distortedview

* WIP: fix tests for augment and operations

* drop julia 0.7 and update CI

* use FileIO v1.1.0

JuliaIO/FileIO.jl#245

* add 1.x tests to CI

* revert back unnecessary changes

* test windows in travis

* fix randomly failed test cases for inacuraccy reasons

* update to julia 1.1

* maybe_copy doesn't explain its usage
* replace Slice by OffsetArrays.IdentityUnitRange

JuliaArrays/OffsetArrays.jl#62

* update to julia 1.2 -- part I

* WIP: update to julia 1.2 -- part II

detect some type instable test cases, only the outmost wrapper is
instable here

* unify how types are displayed

Only meta operations such as Either, Pipeline doesn't have Agumentor
prefix

* add more test versions

* move safe_rand to compat.jl

rand is thread-safe in julia 1.3

* don't allow failures for julia 1.3

* fix commit "WIP: update 1.0"

* use explicit and intuitive CartesianIndex(1, 1)

* add method specialization for tweight

* allow FileIO 1.2

* fix type instability for Either

* update reference for FixedPointNumbers v0.7

* try: relax equality check for scale

* restore test cases in tst_scale.jl

* Revert "add method specialization for tweight"

This reverts commit a698382.

* Augmentor v0.6.0-pre
johnnychen94 added a commit to Evizero/Augmentor.jl that referenced this pull request Jan 29, 2020
@johnnychen94
update codebase to 1.0 and 1.x (continue #27) (#31)
70111ac 
* add Project.toml

* update test for cache and minor fix

* update test for rotation and minor fix

* update flipdm to reverse

JuliaLang/julia#26369

* update test for crop and minor fix

* update test for resize

* update test for scale and minor fix

* update test for zoom and minor fix

* WIP: update test for distortion and minor fix

* update test for either and minor fix

* fix pipeline test

* update summary reference for distortedview

* WIP: fix tests for augment and operations

* drop julia 0.7 and update CI

* use FileIO v1.1.0

JuliaIO/FileIO.jl#245

* add 1.x tests to CI

* revert back unnecessary changes

* test windows in travis

* fix randomly failed test cases for inacuraccy reasons

* update to julia 1.1

* maybe_copy doesn't explain its usage
* replace Slice by OffsetArrays.IdentityUnitRange

JuliaArrays/OffsetArrays.jl#62

* update to julia 1.2 -- part I

* WIP: update to julia 1.2 -- part II

detect some type instable test cases, only the outmost wrapper is
instable here

* unify how types are displayed

Only meta operations such as Either, Pipeline doesn't have Agumentor
prefix

* add more test versions

* move safe_rand to compat.jl

rand is thread-safe in julia 1.3

* don't allow failures for julia 1.3

* fix commit "WIP: update 1.0"

* use explicit and intuitive CartesianIndex(1, 1)

* add method specialization for tweight

* allow FileIO 1.2

* fix type instability for Either

* update reference for FixedPointNumbers v0.7

* try: relax equality check for scale

* restore test cases in tst_scale.jl

* Revert "add method specialization for tweight"

This reverts commit a698382.

* Augmentor v0.6.0-pre
cmcaine added a commit to bovine3dom/Nauty.jl that referenced this pull request Mar 26, 2022
@cmcaine
Use reverse instead of flipdim
5355068 
flipdim was deprecated: JuliaLang/julia#26369
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan left review comments

@mbauman mbauman mbauman approved these changes

Assignees
No one assigned
Labels
arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.
5 participants
@JeffBezanson @StefanKarpinski @stevengj @mbauman @ararslan