Naming of CSR/CSC-related things (LayoutCS and the CS record)

[bradcray]

In past PRs, we've converted most of our standard user-facing distributions from classes to records, such that:

• class Block defined in the BlockDist module became record blockDist
• class Cyclic defined in the CyclicDist module became record cyclicDist

and so on… Notably missing in that effort were the layouts, particularly the CSR/CSC layout used for sparse computation as an alternative to the default COO layout. We simply didn't get to it due to lack of time. In #25812, I'm working on making up for that.

This issue asks what we should name the module and layout record going forward. Sparse is still an unstable feature in Chapel, so technically we don't need to stabilize these names now, but if we get them right now, it'll be fewer changes later as sparse becomes stable.

Currently, the module is named LayoutCS and the class is simply named CS (with arguments, such as compressRows=true or compressRows=false to select between CSR or CSC).

Q1: What should we name the module?

We could retain the current name, but I was noticing that it swaps the order of the Distribution/Layout classification compared to the distributions, so might be more consistent to take this opportunity to change it to CSLayout or CsLayout or CompressedSparseLayout or … This also _may_help with transitioning code between the old and the new, though that shouldn't be a primary consideration.

Q2: What should we name the record?

If we simply changed the current name to a style guide appropriate name, we could just change it to cs. If we followed the approach of the distributions, we could name it csLayout or compressedSparseLayout or layoutCS or whatever the de-capitalized version of the module name is.

Q2a: Or, should we not expose the record name directly, only through aliases?

Users in the field will be most familiar with CSR/CSC as concepts, not some sort of cs(rows=true) initializer like the one we have now. So we could also imagine not worrying about the name of the record and just coming up with names for the aliases to the record, like:

• 'csr', 'csrLayout', 'CSR' (technically a style guide violation)
• 'csc', 'cscLayout', 'CSC' (" " ")

[I was originally thinking of this as a nicety to put on top of the record, so not necessary to discuss from the start, but Engin pointed out below that we don't have to tell the users what the record name is, which is a great point].

[jeremiah-corrado]

My inclination is to make things as symmetric as possible with the naming scheme used for the distributions. So, I'd be in favor of changing to CsLayout for the module name and csLayout for the type name.

Note that I like CSLayout better than CsLayout for the module name and would be okay with making an exception to the standard module style guide for that case (like we did for locale.numPUs, as noted here)

[e-kayrakli]

I came here to push my own agenda :p In general, I am not a big fan of "CS" or "Compressed Sparse" as standalone terms in the interface because I don't see them used like that elsewhere. Not that there are many languages with layouts, but still.

Q1: What should we name the module?

My ideal choice is to have SparseLayouts. We have distribution-specific modules, and my suggestion diverges from that. That being said, layouts are not technically limited to sparse ones (a col-major layout would be a non-sparse alternative). Moreover, there could be a lot of similarities between sparse layouts which could make implementation easier if they are on the same module. Finally, I can imagine different sparse layouts being used in conjunction with one another, whereas the same story is not that common for distributions.

This avoids using "CS" etc in the name as I mentioned above.

But if this is more than intended at the moment, then, I am for CsLayout > CompressedSparseLayout (EDIT I like CompressedSparseLayout better now. #25819 (comment)) . CSLayout is wrong per our style guide. We renamed GPUDiagnostics to GpuDiagnostics, for example. (Typed this part before seeing @jeremiah-corrado's comment)

Q2: What should we name the record?

I want to hide the record behind type aliases if possible. So, I would choose any name for the record. Then would have

type csrLayout = something(compressedRow=true, ?);  // I can't recall if there are other fields
type cscLayout = something(compressedRow=false, ?); // ? may be unnecessary

as the user-facing interface for types.

As implied by my name choices above; if this is too big of a change at this moment, I would go for csLayout.

[bradcray]

Note that I like CSLayout better than CsLayout for the module name and would be okay with making an exception to the standard module style guide for that case

I tend to agree. I've been wondering whether the standard module style guide should simply be updated to permit an identifier that starts with an acronym to use all-caps for that acronym and the next word so that this wouldn't be an exception. Basically extend the "is the entire symbol name" exception to "is the first part, or entire, symbol name" (similarly, it seems like the numPUs exception could be made non-exceptional about "if the natural spelling of the 'word' contains a mix of upper- and lower-case letters, that mix can be preserved").

[bradcray]

My ideal choice is to have SparseLayouts.

Does that suggest that all future (standard) sparse layouts would need to be added to this module? (e.g., say I wanted to add a tridiagonal/k-diagonal layout of sparse layout that stored dense 2D blocks). I worry that would be pretty non-scalable in terms of code maintenance and compile times.

there could be a lot of similarities between sparse layouts which could make implementation easier if they are on the same module.

The similar code could also be refactored into a helper module without the user being any the wiser though (and as you probably know, this already happens with our COO and CSR/CSC sparse layouts… though in some cases I worry it goes a little too far).

I want to hide the record behind type aliases if possible.

Yeah, I want to do that as well (though I was thinking about going full to csr/csc), though that felt like it was orthogonal to the critical questions here, so I didn't bring it up. But I see that your point is that it's not critical if we choose not to tell teh user about the record type. Let me edit the OP to indicate that as a possibility now that you've made it one for me.

[e-kayrakli]

I've been wondering whether the standard module style guide should simply be updated to permit an identifier that starts with an acronym to use all-caps for that acronym and the next word so that this wouldn't be an exception. Basically extend the "is the entire symbol name" exception to "is the first part, or entire, symbol name" (similarly, it seems like the numPUs exception could be made non-exceptional about "if the natural spelling of the 'word' contains a mix of upper- and lower-case letters, that mix can be preserved").

I was involved in the exact same design discussion and voted alongside what you're thinking. However, majority was very clearly in the other direction. In my recollection, CSR/CSC didn't came up in that discussion heavily, but HTML was used as a similar example. Argument for not adopting the style you refer to was when you have something like CSRLayout, it is not very clear how many words/abbreviations there are and where they begin as CSRL could be seen as jumble of characters. I tend to agree that that's a good argument, and come to like the decision we made in that discussion. I believe there was relatively equal amount of weight on both sides of the design spectrum when it comes to other languages' style guides. So, where we are is not really bizarre at all. With that background, I am for sticking to the current style guide. IOW, I prefer CsLayout over CSLayout.

Does that suggest that all future (standard) sparse layouts would need to be added to this module? (e.g., say I wanted to add a tridiagonal/k-diagonal layout of sparse layout that stored dense 2D blocks). I worry that would be pretty non-scalable in terms of code maintenance and compile times.

I can see that it could be problematic maintenance-wise, but I don't think we'll add that many standard sparse layouts ever. Those could come in package/mason modules in my view. I was mostly thinking about BCSR/BCSC versions of these layouts.

I am not too sure about the impact on compilation times, which would be more worrisome. It feels like most of the unnecessary stuff will be dropped by scope resolution. I am not very confident in this assessment.

The similar code could also be refactored into a helper module without the user being any the wiser though (and as you probably know, this already happens with our COO and CSR/CSC sparse layouts… though in some cases I worry it goes a little too far).

That is true. Without delving too much into specifics, I agree that this is mostly an implementation detail rather than an interface design.

[e-kayrakli]

Thinking about it some more -- I am liking CompressedSparseLayout more and more, if do not go with SparseLayout. It is obvious now, but they are almost the same thing while the former can cover for BCSR etc cases as I'd like while leaving Brad's tridiagonal layouts in their own modules. Implying much less worrisome code scalability challenges.

I like the name spelled out as CsLayout is not at all clear as per my statement about "cs" not being used as standalone terms anywhere.

[ShreyasKhandekar]

My initial thoughts here align with what Engin suggested. I liked SparseLayout the most since it is smaller than CompressedSparseLayout but I also think the latter is more descriptive of what kind of layouts we have in the module.

Now, I also think that SparseLayout might be too broad of a name because it might suggest that BSR/DIAgonal/DictionaryOfKeys and all the other sparse layouts also belong in this module. I would restrict the module to just CSC/CSR and add other implementations elsewhere if we want to. But that's implementation details, not relevant.

Moreover, cs(rows=true) seems a roundabout way making a CSR layout (especially because CSR is a common term used to describe this) and that's even more true for cs(rows=false) for CSC.

I would like to go with the suggestion in 2a in the OP to not expose the name of the CS record and only use type aliases which mention the words csr/csc.

[jeremiah-corrado]

I would like to go with the suggestion in 2a in the OP

I'd also be in favor of this option.

[bradcray]

@jeremiah-corrado , @e-kayrakli , @ShreyasKhandekar : Belatedly, thanks for the quick responses here, and what feels like a potential consensus path forward. Here's what I'm currently implementing, where I'm looking for feedback along the lines of:

• Could we make the following change / adjustment: …
• This looks good for 2.2!
• I don't think we should live with this question for more time before we proceed (i.e., don't make any changes here for 2.2)

1. I've added a new CompressedSparseLayout module to replace the LayoutCS module

2. It contains a no-doc'd record csLayout that is the closest equivalent to the old CS class and used to implement the user-facing layouts. I originally named this compressedSparseLayout, but that name got old fast and since it isn't user-facing, I took the shorter name

3. It also contains type aliases, csrLayout and cscLayout which are implemented in terms of csLayout.

4. The LayoutCS module is retained, but generates a deprecation message when used:
   'LayoutCS' and the 'CS' layout are deprecated; please use 'CompressedSparseLayout' and 'csrLayout' or 'cscLayout' instead

5. The CS class is retained, but generates a deprecation message when used:
   'CS' is deprecated, please use 'CompressedSparseLayout.[csrLayout|cscLayout]' instead

I've got enough testing passing with this new approach that I feel pretty confident that I could plumb it through all remaining tests; but if we were to rename any of the new things, that'd be good to settle before doing that full conversion.

Thanks!

[jeremiah-corrado]

IMO, this looks good for 2.2!

[e-kayrakli]

This looks good for 2.2!

[ShreyasKhandekar]

Just so I understand the user facing side of this change properly and to also make sure we see it to make sure it looks right. Making this change would have users writing code that looks like the following:

use CompressedSparseLayout;
var parentDom = {1..100,1..100};
var cscSparseDom: sparse subdomain(parentDom) dmapped new cscLayout(somethingGoesHere?); 
var csrSparseDom: sparse subdomain(parentDom) dmapped new csrLayout(somethingGoesHere?); 

OR, do we still have to use the dmap type with these layouts?

If it's the first, I'm for making these changes for 2.2!

[bradcray]

@ShreyasKhandekar : You're correct. Ultimately, dmapped will be replaced with… whatever we decided to replace it with (I can never recall), but this will be the syntactic pattern. No arguments are required where you have somethingGoesHere?, but there is an optional argument that lets you specify whether you want to keep the indices in a given row/col sorted or not (some CSR/CSC implementations do, others don't).

[bradcray]

Though I didn't land the CSR/CSC renaming and recordization effort in time for 2.2 (so close…), I did manage to wrap it up now that release notes are out and so am now looking for a reviewer for #26137 where I think someone from the crew on this issue (@jeremiah-corrado , @e-kayrakli , @ShreyasKhandekar ) would be best-suited to review it.

Note that the size of the patch is large, but that much of the code in CompressedSparseLayout.chpl is a properly indented clone of LayoutCS.chpl and doesn't need to be (re-)reviewed at this point. I suspect being a little clever with diff commands would permit you to focus on the relevant parts (which should mostly be the record types).

Size: 4/7 if we take the code clone into account, more like 6/7 otherwise
Complexity: 3/7
Pride: 5/7

[ShreyasKhandekar]

I can take a look soon, unless somebody beats me to it :)