Provide a way for sets to zipper with more things #26595
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a pair of leader/follower iterators to ChapelHashtable and a helper method, and calls them from the Set `these` iterators. The implementation does frequent recomputation of how to divide up the iteration space to find full slots. This can probably be improved on, but I wanted to take a snapshot of what worked before attempting it. It also does not help when zippering with arrays yet. That is also a next step ---- Signed-off-by: Lydia Duncan <[email protected]>
Test comes from issue chapel-lang#15824 and checks zippering two sets of the same length (but different contents). Sorts the output so that it is consistent instead of potentially racy. ---- Signed-off-by: Lydia Duncan <[email protected]>
---- Signed-off-by: Lydia Duncan <[email protected]>
This is a first step towards being able to zip sets with other types instead of just other sets ---- Signed-off-by: Lydia Duncan <[email protected]>
This meant always computing the number of chunks and which chunk we were ourselves, since the array iterator expects the other components of the tuple to be ranges as well (so we can't use followThis to send that information) and the argument list for the follower is supposed to match the argument list of the leader aside from followThis. This is unfortunate, but given the sizes I checked, seemed to be okay. ---- Signed-off-by: Lydia Duncan <[email protected]>
Covers: - converting the set to an array and then zippering - zippering with the array leading - zippering with an array when the set leads With the prior commit, all three work (though toArray already worked prior to this effort). ---- Signed-off-by: Lydia Duncan <[email protected]>
The original is only failing on one of the two machines I checked, so I'm curious if this also fails there or if it behaves as expected (it passes on the machine where the original still works) ---- Signed-off-by: Lydia Duncan <[email protected]>
---- Signed-off-by: Lydia Duncan <[email protected]>
I'm not sure if this will actually allow us to check for unequal lengths, but try it and see ---- Signed-off-by: Lydia Duncan <[email protected]>
This argument indicates the size of the leader in the iteration, so that we can validate if the iteration space is uneven. It has a default value so that the non-zippered case is not as annoying to write Update various tests to use this iterator instead ---- Signed-off-by: Lydia Duncan <[email protected]>
- document new iterator - remove commented out writelns - restore old behavior to Set's these iterators, since we're using a different iterator - comment an if statement in _determineEvenChunks - close a parenthetical in an older comment in ChapelHashtable ---- Signed-off-by: Lydia Duncan <[email protected]>
It'll present a cleaner interface to users ---- Signed-off-by: Lydia Duncan <[email protected]>
Covers: - Zippering with a shorter array as leader - Zippering with a shorter array as follower - Serial version of the new iterator - Standalone version of the new iterator ---- Signed-off-by: Lydia Duncan <[email protected]>
---- Signed-off-by: Lydia Duncan <[email protected]>
…f it Would have added a warning to all the overloads, except for the bug reported in all the right contexts, whether the bug or the unstable warning gets resolved first. ---- Signed-off-by: Lydia Duncan <[email protected]>
Use a halt wrapper instead of `__primitive("chpl_error"`, especially because
this is a new error instead of trying to match errors that the compiler can
insert.
While doing that, made the error messages clearer about what was wrong and what
should be done instead (and hopefully head off confusion for the follower case).
And add a test locking in the error message in the leader case
----
Signed-off-by: Lydia Duncan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this work, sets could only zipper with identical other sets (or basically just itself). Zippering with sets of the same length, or with arrays, would result in errors about unequal zippering lengths.
This PR adds a new unstable iterator to Set,
contents. This iterator is far from perfect but is a good start at providing this functionality.Adds a leader/follower pair of iterators to ChapelHashtable to support this effort. These iterators and their support functions allow the hashtable to evenly divide the elements stored in it, instead of evenly dividing the entire hashtable's storage capacity. They make some explicit error calls like normal zippering does to try and keep the error messages in line with the ones generated by the compiler, though I strongly suspect I may have missed some.
While here, I fixed a comment that was missing a closing parenthesis.
Test updates:
these, as it demonstrated that we need to be passed the size of the leader or we'd fail to detect zippering size mismatchesNew tests:
toArrayresult (user code, but not expected to have changed as a result of this effort)Places for improvement:
@unstablewhile implementing this (see [Bug]: Unstable iterator overloads always warn, even if another overload is used #26590). It'd be nice to resolve that, but not essential for this PR to function.Passed a full paratest with futures