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

potentially confusing behavior in cartesian loops #330

Closed
StefanKarpinski opened this issue Jan 6, 2012 · 23 comments · Fixed by #26483
Closed

potentially confusing behavior in cartesian loops #330

StefanKarpinski opened this issue Jan 6, 2012 · 23 comments · Fixed by #26483
Assignees
Labels
breaking This change will break code compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Milestone

Comments

@StefanKarpinski
Copy link
Member

Not sure if this is a bug, but it did surprise me:

julia> for x = int64(2)^53-2:int64(2)^53+5, y=1:2
         println(x)
         x = uint64(x)
       end
9007199254740990
0x001ffffffffffffe
9007199254740991
0x001fffffffffffff
9007199254740992
0x0020000000000000
9007199254740993
0x0020000000000001
9007199254740994
0x0020000000000002
9007199254740995
0x0020000000000003
9007199254740996
0x0020000000000004
9007199254740997
0x0020000000000005
@ghost ghost assigned JeffBezanson Jan 6, 2012
@vtjnash
Copy link
Member

vtjnash commented Jan 6, 2012

thought this might be a little clearer (if I understand the bug/unexpected behavior correctly)

julia> for x = 1:10, y=1:2
               println(x," ", y)
       end
1 1
1 2
2 1
2 2
3 1
3 2
4 1
4 2
5 1
5 2
6 1
6 2
7 1
7 2
8 1
8 2
9 1
9 2
10 1
10 2

@JeffBezanson
Copy link
Member

I don't see what else this could do.

@StefanKarpinski
Copy link
Member Author

@vtjnash: That's actually what it's supposed to do. It's a shorthand for a pair of nested for loops.

@JeffBezanson: I guess why I found it surprising is that we're used to having loop variables reset on each iteration, regardless of what we do with them. In this case, however, x iterations and y iterations are not the same, despite them appearing to be in the same "superloop". It really is just syntax for nested for loops. It could mean something like this instead:

for _x = int64(2)^53-2:int64(2)^53+5, _y=1:2
  x = _x; y = _y
  println(x)
  x = uint64(x)
end

@JeffBezanson
Copy link
Member

My worry is that somebody might rewrite one form to the other, thinking it is just a syntax shortcut. So the behavior shouldn't change.

@StefanKarpinski
Copy link
Member Author

Let's just leave this behavior as-is and close this issue.

@cmey
Copy link
Contributor

cmey commented Dec 4, 2013

This hit me today, I was doing something like this in image processing:

for x=1:4,y=1:2
       #processing using x,y
       println( (x,y) );
       (y,x) = (x,y)
       #processing using transposed x,y
end

The behavior of the cartesian loop syntax was very confusing.
Now I understand that if I had written two separate loops, that behavior would have been expected. But in the cartesian loop syntax, I expected the variables x and y to be reset, like @StefanKarpinski said.

@rfourquet
Copy link
Member

This was closed in 2012, but in the meantime, the break behavior has changed from within "multiloops" (with one for keyword), in #7430. Is it worth reconsidering this issue?

@JeffBezanson
Copy link
Member

Yes, that's a good point. Either option seems ok to me; I'll sleep on it. Would be interesting to see what others think.

@StefanKarpinski
Copy link
Member Author

I can see it either way, but I still think my original example is a bit of a gotcha.

@rfourquet
Copy link
Member

The comprehensions seem to suggest a change:

julia>  [let z=(x, y); x=0; z end for x=1:2 for y=1:2]
4-element Array{Tuple{Int64,Int64},1}:
 (1, 1)
 (0, 2)
 (2, 1)
 (0, 2)

julia>  [let z=(x, y); x=0; z end for x=1:2, y=1:2]
2×2 Array{Tuple{Int64,Int64},2}:
 (1, 1)  (1, 2)
 (2, 1)  (2, 2)

@StefanKarpinski
Copy link
Member Author

I do think there's a solid argument to be made there:

  • for comprehensions we really want each body to be evaluated completely independently
  • we want for loops to behave similarly to for loops

Therefore, we should change this. We can argue with each point, but if we accept both premises, that seems to be a solid argument.

@rfourquet
Copy link
Member

Therefore, we should change this.

Do you mean changing the comprehensions with 2 for? I will also sleep on that, but I find that there is a nice simple mental model which makes comprehensions equivalent to for loops: [f(x...) for (x...) = ... if ...] always desugars to

result = [] # forgetting about array type
for x... = ... # nested for or cartesian for
    if ...
        push!(result, f(x...))
    end
end

So there is also an argument to be made to keep this double equivalence between for loops and comprehensions (by "double" I mean "nested for loop" <=> "nested for in comprehension" on one hand, and "cartesian for loops" <=> "cartesian comprehension" on the other hand).

@rfourquet
Copy link
Member

Adding triage here, as no definitive conclusion was reached since last discussion in September.

@rfourquet rfourquet reopened this Dec 7, 2017
@rfourquet rfourquet added the triage This should be discussed on a triage call label Dec 7, 2017
@StefanKarpinski
Copy link
Member Author

That's a different issue... the behavior shown here does not happen anymore:

julia> for x = Int64(2)^53-2:Int64(2)^53+5, y=1:2
           println(x)
           x = UInt64(x)
       end
9007199254740990
9007199254740990
9007199254740991
9007199254740991
9007199254740992
9007199254740992
9007199254740993
9007199254740993
9007199254740994
9007199254740994
9007199254740995
9007199254740995
9007199254740996
9007199254740996
9007199254740997
9007199254740997

@StefanKarpinski
Copy link
Member Author

@rfourquet, if there's some other issue here, please open a new issue.

@vtjnash vtjnash removed triage This should be discussed on a triage call needs decision A decision on this change is needed labels Dec 7, 2017
@vtjnash vtjnash removed this from the 1.0 milestone Dec 7, 2017
@cmey
Copy link
Contributor

cmey commented Dec 8, 2017

The behavior shown here has been fixed when messing with the outer loop only, but still happens when messing with both loop indices:

julia>for x=1:4, y=1:2
          println( (x,y) )
          (y,x) = (x,y)
       end
(1, 1)
(1, 2)
(2, 1)
(1, 2)
(3, 1)
(1, 2)
(4, 1)
(1, 2)

Expected (x and y reset):

(1, 1)
(1, 2)
(2, 1)
(2, 2)
(3, 1)
(3, 2)
(4, 1)
(4, 2)
julia> versioninfo()
Julia Version 0.6.1
Commit 0d7248e (2017-10-24 22:15 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin14.5.0)
  CPU: Intel(R) Core(TM) i7-4980HQ CPU @ 2.80GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, haswell)

@martinholters
Copy link
Member

the behavior shown here does not happen anymore

But that is only because println(::UInt64) output looks like println(::Int64) output now, no? The underlying issue is still the same:

julia> for x = Int64(2)^53-2:Int64(2)^53+5, y=1:2
           println(typeof(x))
           x = UInt64(x)
       end
Int64
UInt64
Int64
UInt64
Int64
UInt64
Int64
UInt64
Int64
UInt64
Int64
UInt64
Int64
UInt64
Int64
UInt64

@StefanKarpinski
Copy link
Member Author

Ah, I see. Well, I could have sworn this was fixed.

@JeffBezanson JeffBezanson added compiler:lowering Syntax lowering (compiler front end, 2nd stage) triage This should be discussed on a triage call labels Mar 9, 2018
@JeffBezanson JeffBezanson added this to the 1.0 milestone Mar 14, 2018
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Mar 14, 2018
@JeffBezanson
Copy link
Member

Puzzler: if I write

local i
for outer i = 1:2, j = 1:0
end

does i get set?

@JeffBezanson
Copy link
Member

A related issue is that these loops are not really "cartesian"; a loop iterator can depend on an outer variable, as in for i = 1:n, j = 1:i. In that sense the j loop really is nested inside the i loop, like generators of the form for i = A for j = B, whose behavior they match.

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Mar 15, 2018
@StefanKarpinski
Copy link
Member Author

StefanKarpinski commented Mar 15, 2018

This currently matches what comprehensions do:

julia> [(t=(i,j); i=nothing; t) for i = 1:3 for j = 1:i]
6-element Array{Tuple{Any,Int64},1}:
 (1, 1)
 (2, 1)
 (nothing, 2)
 (3, 1)
 (nothing, 2)
 (nothing, 3)

where as it could mean this:

julia> [let i=i, j=j; (t=(i,j); i=nothing; t); end for i = 1:3 for j = 1:i]
6-element Array{Tuple{Int64,Int64},1}:
 (1, 1)
 (2, 1)
 (2, 2)
 (3, 1)
 (3, 2)
 (3, 3)

@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Mar 15, 2018
@JeffBezanson
Copy link
Member

OK, decided we need to change both, due to this correspondence.

@StefanKarpinski
Copy link
Member Author

Triage has badgered @JeffBezanson into agreeing to change this for both compound for loops and chained comprehensions.

cmcaine pushed a commit to cmcaine/julia that referenced this issue Nov 11, 2022
* Add file patterns to config.json file

* Apply suggestions from code review

Co-authored-by: Sascha Mann <[email protected]>
Keno pushed a commit that referenced this issue Oct 9, 2023
This is a temporary workaround for #330
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants