Add some new sparse methods based on feedback #26280
Conversation
--- Signed-off-by: Brad Chamberlain <[email protected]>
…rse-methods-for-rich
--- Signed-off-by: Brad Chamberlain <[email protected]>
bradcray
changed the title
There was a problem hiding this comment.
At first glance, those names seem good to me! So hopefully a stabilized version wouldn't need to make changes.
It'd be great to either add these functions to a test in the test/unstable/ directory or add a test of them there, so that we have tracking of them being unstable and maybe don't accidentally stabilize them when working to stabilize sparse as a whole (like we've done with other things). Other than that and the question about one of the test outputs, I think this is good to go!
In reviewing my PR, Lydia noticed something I hadn't: That the coordinates array had some extra entries at the end, due to the fact that we use recursive doubling on the nnzDom domain such that it isn't always exactly nnz elements large. Here, I've addressed that by resizing the domain to be just the right size before returning the references to the coordinates. --- Signed-off-by: Brad Chamberlain <[email protected]>
I feel disinclined to do this from the perspective of "everything that involves sparse is unstable / hasn't been reviewed", and not wanting to replicate all features that the sparse test directories currently exercise separately in the unstable directory (from the perspective of duplicate testing). My thought here is that we'd review all sparse methods when we review and stabilize sparsity in general, particularly given that we don't even really have chpldocumentation for sparse methods or features currently (nor, so far as I can tell, an established way to generate them given how specialized the docs around distributions and layouts are?) So basically, I think of the actions for stabilizing sparse as being:
Is that OK with you? I can capture this concern and proposal of a plan in an internal issue called something like "stabilize sparse domains/arrays and their methods" instead (assuming we don't already have one) if that sounds acceptable. [edit: We can also work this after feature freeze, which for me is today] |
--- Signed-off-by: Brad Chamberlain <[email protected]>
|
The updated code looks good, and I'm okay with recording that plan in an internal issue. Thanks! |
I've put that here: https://github.com/Cray/chapel-private/issues/6917 |
While at CLSAC, I had the chance to meet up with Rich Vuduc, who had done a little algorithmic tinkering with sparse arrays in Chapel this past year in his spare time. He mentioned some helper routines that he had wanted, and ended up writing himself, digging into Chapel's internals. In this PR, I've added versions of three of the main routines he was missing to the relevant domain maps:
A 'getCoordinates()' method on default/COO sparse arrays that returns a [a reference to] an array of tuples representing the row/col coordinates of the nonzeroes (for a 2D sparse array, or a k*tuple for kD arrays).
A zero-argument 'getLocalSubarray()' query that returns [a reference to] an array representing the current locale's local chunk of a block-distributed sparse array.
A 'localSubarrays()' iterator that iterates over all of a block-distributed sparse array's target locales, yielding the local subarray on the corresponding locale. So this could be used to spin up SPMD-style parallelism with a task per locale and a reference to that locale's block. There's also a serial version that iterates over the locales, yielding their blocks serially (mostly for debugging and completeness; seems less likely to be used in practice when perf matters)
While here, this also adds
refoverloads for some of the existing utility routines that only providedconst refversions before, to lean on ref intent overloading and permit their contents to be modified directly. This was another thing that came out of the conversation with Rich.I'm not sure these are the best/right names, but since sparse is unstable, also didn't labor over them very much, thinking it would be best to get them into users' hands and get feedback on their utility.