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

inconsistent results when broadcasting with empty segments on multilocale #3035

Closed
stress-tess opened this issue Mar 13, 2024 · 2 comments · Fixed by #3039
Closed

inconsistent results when broadcasting with empty segments on multilocale #3035

stress-tess opened this issue Mar 13, 2024 · 2 comments · Fixed by #3039
Assignees
Labels
bug Something isn't working User Reported A user submitted the issue

Comments

@stress-tess
Copy link
Member

A user reported this and I was able to reproduce. Below is what I'm seeing with gasnet and 3 locales

>>> segments = ak.array([3,3,5,6,6,7,7])
>>> values = ak.arange(7)
>>> size = 10
>>> ak.broadcast(segments, values, size)
array([0 0 0 1 1 2 3 4 4 4])

>>> ak.broadcast(segments, values, size)
array([0 0 0 1 1 2 3 4 4 4])

>>> ak.broadcast(segments, values, size)
array([0 0 0 0 0 1 2 3 3 3])
@stress-tess stress-tess added bug Something isn't working User Reported A user submitted the issue labels Mar 13, 2024
@stress-tess
Copy link
Member Author

This unexpected result is due to a race condition in this forall loop:

arkouda/src/Broadcast.chpl

Lines 151 to 153 in 9ab6f17

forall (s, d) in zip(segs, diffs) with (var agg = newDstAggregator(t)) {
agg.copy(expandedVals[s], d);
}

if we print out the values for segs and diffs we get:

segs: 3 3 5 6 6 7 7
diffs: 0 1 1 1 1 1 1

we see that the seg 3 has two associated diffs, so expandedVals[3] will vary depending on the order that these two segments write

@stress-tess
Copy link
Member Author

I believe the correct result for this case should be array([0 0 0 1 1 2 4 6 6 6])

My reasoning is:

  • we don't specify what goes in (0,3) so it remains uninitialized (i.e. 0)
  • 0 goes in segment (3,3) which is empty
  • 1 goes in segment (3,5)
  • 2 goes in segment (5,6)
  • 3 goes in segment (6,6) which is empty
  • 4 goes in segment (6,7)
  • 5 goes in segment (7,7) which is empty
  • 6 goes in segment (7,10)

stress-tess added a commit to stress-tess/arkouda that referenced this issue Mar 15, 2024
…pty segments

This PR (fixes Bears-R-Us#3035) a race condition bug that caused inconsistent results when broadcasting with empty segments (though both results given were incorrect).

We get around this by masking out the segments and values associated with the empty segments. We only do this when empty segments exist, so it shouldn't affect the performance of the common case too much but will give correct results in the empty seg case.
@stress-tess stress-tess self-assigned this Mar 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Mar 18, 2024
…nts (#3039)

* Fixes #3035 - Inconsitent results when broadcasting with empty segments

This PR (fixes #3035) a race condition bug that caused inconsistent results when broadcasting with empty segments (though both results given were incorrect).

We get around this by masking out the segments and values associated with the empty segments. We only do this when empty segments exist, so it shouldn't affect the performance of the common case too much but will give correct results in the empty seg case.

* had to add in empty check to avoid a segfault on one of the coargtests

---------

Co-authored-by: Tess Hayes <stress-tess@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working User Reported A user submitted the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant