chapel icon indicating copy to clipboard operation
chapel copied to clipboard

Change CSR/CSC layout class 'CS' to `csrLayout`/`cscLayout` records and rename `LayoutCS` module

Open bradcray opened this issue 6 months ago • 0 comments

As part of our ongoing conversion of domain maps from classes to records, this PR changes the CS class that has been used to represent sparse CSR/CSC domains and arrays into a pair of records, csrLayout and cscLayout. Simultaneously, it renames the LayoutCS.chpl module to CompressedSparseLayout.chpl to (a) move Layout from a prefix to a suffix, similar to the other distributions and (b) make the name a bit more self-descriptive (since 'CS' isn't generally recognized as being the prefix of CSR/CSC). This approach also made it easy to generate reasonable deprecation warnings for the old approach by essentially starting from a code clone of the LayoutCS module and attaching @deprecated attributes to the module and class declarations.

Despite these improvements, I've left the new module and record type as @unstable since sparse as a whole is unstable and the routines used here haven't been reviewed in any detail.

With this change, I believe DefaultDist() is the only class-based domain map remaining and the only thing standing in the way of removing the dmap type (?).

Most of the diff here is updating module and test code from dmapped new dmap(new CS()) to one of dmapped new csrLayout() or dmapped new cscLayout(), preserving the sortedIndices argument when used.

For the implementation of the record itself, I took a similar approach to the records defining blockDist, cyclicDist, etc. by preserving the old classes and simply wrapping them in the record, combined with using a helper record as a means of providing common capabilities between the csrLayout/cscLayout record types and, hopefully, DefaultDist() once it's converted to a record.

Originally, I'd hoped to use a single csLayout record to define both csrLayout and cscLayout as partial instantiations of it, but I had problems coming up with an approach that caused the two types to work as both a fully-defaulted type (needing no arguments) and a generic type that could accept them. The results of these failed attempts are noted in comments, as I think that's worth returning to… In part to remove a bit of repetition from the code, and in part because it feels like an important case for the language to be able to support. I'll also plan to file an issue or issues about what I found there.

As a result, CompressedSparseLayout.chpl is essentially:

  • a code clone of LayoutCS.chpl
  • promoted the module from a prototype to a true module by adding throws decorators to the dsiSerialWrite() calls
  • renamed LayoutCSDefaultToSorted to csLayoutDefaultToSorted to better math the module name
  • updated the isCSType() query to work with the new records
  • changed the CS class into a CSImpl class that the records wrap
    • removed its default for compressRows in the process because it's unnecessary and could just mask mistakes
  • added a chpl_layoutHelper record that is similar to the chpl_distHelp record used for the Block, Cyclic, etc. conversions, whose goal is to factor things that used to be inherited from BaseDist into a common record that could be used by multiple types; currently it's only this one, but maybe DefaultDist will be able to make use of it as well? Or maybe it's overkill / putting the cart before the horse.
  • added the csrLayout / cscLayout records, which simply define initializers and == / != operators
  • preserved the aforementioned unsuccessful attempt to define a csLayout type that could be instantiated twice to remove some of the code duplication involved in having two records above
  • added a dsiGetDist() call to convert a class back into a distribution record value

Taking stock of other changes required:

  • modules/layouts/LayoutCS.chpl: marked the module and CS class as deprecated
  • modules/Makefile, fixDistDocs.perl: added new entry to ensure that the new module is documented
  • dists/BlockDist.chpl, dists/SparseBlockDist.chpl, packages/LinearAlgebra.chpl: updated to use the new module and type names
  • test/*.chpl: For all tests, I essentially converted:
    • uses/imports of LayoutCS into CompressedSparseLayout
    • dmap(new CS()) and dmap(new CS(compressRows=true)) into csrLayout()
    • dmap(new CS(compressRows=false)) into cscLayout()
      • in cases like the above that passed sortIndices, I preserved those arguments
    • references to unmanaged CS into csrLayout
    • cases that chose between the two using a param variable into a conditional that chooses between the types
    • cases that have to support both cs*Layout and DefaultDist to wrap the latter in a dmap+unmanaged and the former not
    • cases that extended the CS class to extend CSImpl instead
  • test/*.compopts: For cases that passed in CS as a config type, I changed to csrLayout

TODO:

  • [ ] check docs and comments
  • [ ] add test locking in deprecations for old module/class
  • [ ] file issues for shared record pattern that didn't work out
  • [ ] look at what's required to keep Arkouda running

bradcray avatar Aug 26 '24 16:08 bradcray