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

CartesianIndices (breaking?) change in 1.6.0-rc1 #39589

Closed
akosuas opened this issue Feb 9, 2021 · 9 comments
Closed

CartesianIndices (breaking?) change in 1.6.0-rc1 #39589

akosuas opened this issue Feb 9, 2021 · 9 comments

Comments

@akosuas
Copy link

akosuas commented Feb 9, 2021

Something has changed here:

Julia Version 1.5.3
Commit 788b2c77c1 (2020-11-09 13:37 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake-avx512)
Environment:
  JULIA_PKG_SERVER = pkg.julialang.org
julia> CartesianIndices((3, 1:3))
1×3 CartesianIndices{2,Tuple{UnitRange{Int64},UnitRange{Int64}}}:
 CartesianIndex(3, 1)  CartesianIndex(3, 2)  CartesianIndex(3, 3)
julia> versioninfo()
Julia Version 1.6.0-rc1
Commit a58bdd9010 (2021-02-06 15:49 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake-avx512)
Environment:
  JULIA_PKG_SERVER = pkg.julialang.org
julia> CartesianIndices((3, 1:3,))
3×3 CartesianIndices{2, Tuple{Base.OneTo{Int64}, UnitRange{Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 2)  CartesianIndex(1, 3)
 CartesianIndex(2, 1)  CartesianIndex(2, 2)  CartesianIndex(2, 3)
 CartesianIndex(3, 1)  CartesianIndex(3, 2)  CartesianIndex(3, 3)
julia> versioninfo()

Is this expected? It is breaking some of my code, and I can't find anything obvious in NEWS that describes this.

@akosuas
Copy link
Author

akosuas commented Feb 9, 2021

Reading a bit more closely, looks likely to be caused by #37829. I wonder if my single-integer usage of CartesianIndices was "supported" in the first place?

@simeonschaub
Copy link
Member

simeonschaub commented Feb 9, 2021

What code is this breaking? CartesianIndex((Base.OneTo(3), 1:3)) does seem more correct to me here than CartesianIndex((1:3, 1:3)). It should behave exactly the same, but IIUC this change was necessary to improve performance of CartesianIndices. Your code might need to be made a little more generic, but since OneTo is an AbstractUnitRange, this should be quite straightforward.

Edit: Sorry, I misread your example, the two behaviors are definitely inconsistent.

@JeffBezanson
Copy link
Member

The 1.5 behavior seems to have been inconsistent:

julia> CartesianIndices((2,3))
2×3 CartesianIndices{2,Tuple{Base.OneTo{Int64},Base.OneTo{Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 2)  CartesianIndex(1, 3)
 CartesianIndex(2, 1)  CartesianIndex(2, 2)  CartesianIndex(2, 3)

julia> CartesianIndices((2,1:3))
1×3 CartesianIndices{2,Tuple{UnitRange{Int64},UnitRange{Int64}}}:
 CartesianIndex(2, 1)  CartesianIndex(2, 2)  CartesianIndex(2, 3)

That can't be right, but I'm not sure what was intended.

@akosuas
Copy link
Author

akosuas commented Feb 9, 2021

Thanks for investigating guys.

If I was relying on some dodgy / broken behaviour in 1.5, I'm happy to switch to an explicit one-element range, i.e. CartesianIndices((3:3, 1:3)) instead of CartesianIndices((3, 1:3).

@mbauman
Copy link
Member

mbauman commented Feb 10, 2021

Yes, that behavior was definitely unintentional and buggy. Integers should always describe 1-based lengths; ranges should always specify indices. Sorry this caught you out — perhaps we should indeed add a NEWS item.

@johnnychen94
Copy link
Member

FYI, I made that change intentionally in #37829 (comment), and also to LinearIndices in #37928.

@akosuas
Copy link
Author

akosuas commented Feb 10, 2021

FYI, I made that change intentionally in #37829 (comment), and also to LinearIndices in #37928.

Ah, nicely done, I hadn't spotted that.

@sostock
Copy link
Contributor

sostock commented Feb 23, 2021

I have added a NEWS/HISTORY item for the change: #39784

mbauman added a commit that referenced this issue Mar 17, 2021
* Add HISTORY.md entry for change noted in #39589

* Rephrase

Following mbauman’s suggestion: #39784 (comment)

Co-authored-by: Matt Bauman <mbauman@gmail.com>
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this issue May 4, 2021
…9784)

* Add HISTORY.md entry for change noted in JuliaLang#39589

* Rephrase

Following mbauman’s suggestion: JuliaLang#39784 (comment)

Co-authored-by: Matt Bauman <mbauman@gmail.com>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this issue May 9, 2021
…9784)

* Add HISTORY.md entry for change noted in JuliaLang#39589

* Rephrase

Following mbauman’s suggestion: JuliaLang#39784 (comment)

Co-authored-by: Matt Bauman <mbauman@gmail.com>
@johnnychen94
Copy link
Member

Close since this is already solved: this change is considered a bug fix with associated NEWS entry.

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