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

Issue an error when a DR domain/array is following a larger leader #17947

Merged
merged 2 commits into from
Jun 19, 2021

Conversation

bradcray
Copy link
Member

@bradcray bradcray commented Jun 18, 2021

Nikhil shared a case on chat this week where—to my surprise—we weren't
generating OOB errors for having a DR array get indexed out of bounds
when it was following a larger leader array. This is something that
we used to catch, but no longer seem to. Here, I'm adding a check to
the DR domain follower code to catch such cases and a new test to lock
the check in.

Note that this doesn't address the longstanding error in which a leader
iterator expression is smaller than its follower (i.e., my most dreaded
Chapel bug (TM): #11428).

Nikhil showed a case on chat this week where—to my surprise—we weren't
generating OOB errors for having a DR array get indexed out of bounds
when it was following a larger leader array.  This is something that
we used to catch, but no longer seem to.  Here, I'm adding a check to
the DR domain follower code to catch such cases.

---
Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
---
Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
@bradcray bradcray marked this pull request as ready for review June 18, 2021 20:33
Copy link
Member

@vasslitvinov vasslitvinov left a comment

Choose a reason for hiding this comment

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

Looks good.

@stonea
Copy link
Contributor

stonea commented Jun 18, 2021

Seems reasonable to error out if that's what we also do in the serial case, which appears to be the case:

var a: [1..4] real;
var b: [1..3] real;

for (i,j) in zip(a.domain,b.domain) {
    writeln(i, j);
}

Errors out with:

11
22
33
foo.chpl:4: error: zippered iterations have non-equal lengths.

I guess it would be even better if we could error out at compile time if that's possible.

FWIW, it looks like Python is fine with this (although I wouldn't say it's necessarily a better decision, unequal zippered lists seem more likely than not to be a bug to me):

>>> a = [1, 2, 3, 4]
>>> b = [1, 2, 3]
>>> for (i,j) in zip(a,b):
...     print (i,j)
... 
(1, 1)
(2, 2)
(3, 3)

@bradcray
Copy link
Member Author

@vasslitvinov, thanks for the review; @stonea, thanks for the notes.

Seems reasonable to error out if that's what we also do in the serial case

It's definitely the intention in Chapel to generally error out for zips of non-matching size / shape, which is why I consider this a bug fix rather than a design decision (in which case, I would've put the discussion before the group better). This case used to result in an error until sometime in the past few years (I didn't bisect to determine precisely when). There are some cases that are exceptions to this "matching size/shape" rule, such as zippering with an unbounded range (e.g., forall (a,i) in zip(myArr, 1..) ...); there's also an intention to give the user the ability to decide whether an iterator needs to match in size or not, though that's future work. (Note that there's also a longstanding bug / limitation of our current framework in which, if the first thing is smaller than the second, we can't currently catch it for a forall loop due to our current design for parallel zippered iteration; we catch either kind of size mismatch in the serial case, which is simpler).

I guess it would be even better if we could error out at compile time if that's possible.

I agree, but this isn't possible in the general case, and we don't have sufficient in the information to catch it today for obvious cases easily.

FWIW, it looks like Python is fine with this

Huh, that's news to me (though it doesn't make me second-guess our decision in Chapel). Thanks for pointing it out.

@bradcray bradcray merged commit 80aed5f into chapel-lang:master Jun 19, 2021
@bradcray bradcray deleted the fix-easy-dr-zip-mismatches branch June 19, 2021 00:34
@bradcray
Copy link
Member Author

@npadmana : This should've reduced the confusion that you experienced in your test on gitter this week. You might still need some help deciphering what happened and why, but at least you wouldn't have gotten a subtle silent failure.

bradcray added a commit to bradcray/chapel that referenced this pull request Jan 27, 2025
I believe that chapel-lang#17947
resolved this.

---
Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
bradcray added a commit that referenced this pull request Jan 27, 2025
[trivial, not reviewed]

I believe that PR #17947 is
the one that resolved this test case.

Resolves #15914
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

Successfully merging this pull request may close these issues.

3 participants