chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.8k stars 423 forks source link

Rename `new*` routines on distributed domains/arrays to `create*` #21196

Closed bradcray closed 1 year ago

bradcray commented 1 year ago

Our current convenience routines for making distributed domains/arrays (e.g., newBlockArr(), newBlockDom()) should be changed to use create() prefixes instead.

[not directly related to this issue, but: We should also review their interfaces for 2.0]

ronawho commented 1 year ago

Not that it matters too much, but I'm pretty sure the current routines are newBlockArr() / newBlockDom()

bradcray commented 1 year ago

D'oh! (I knew there was something I liked about their current names! :D )

Fixed now.

ronawho commented 1 year ago

FWIW I use newBlockDom()/newBlockArr() pretty regularly and they've never bothered me. I'm fine with create* if that's the prefix we've chosen for factory functions but wanted to note that the current ones don't offend me so I wouldn't personally change them except to unify.

That said, I was wondering if they actually need to be factory functions or if they could be initializers on the underlying types (which have the same name) and might look something like new BlockDom(1..10). I'm not sure if that would actually work since we don't just want to create the domain map classes, but also want to then privatize and maybe that's weird to put in the initializer?

bradcray commented 1 year ago

They don't actually bother me either, but my attempts to get us to use newBlah() rather than createBlah() have all failed, and they were called out explicitly in https://github.com/chapel-lang/chapel/issues/14291 which makes me think we need to unify them. The fact that they are used fairly frequently (e.g., Arkouda code, talk slides) that makes me think we should get on this before 2.0.

I also wish for better ways to create these, e.g. through initializers. For me that starts unravelling the sweater of "How user facing is the dmap type?", "Should we be using a keyword other than dmapped?", "How should a distribution value be shared across multiple arrays?" which would be very satisfying to tug at, but definitely has not been on anyone's agenda for 2.0. For me it's been one of those "Seems like it could definitely be much better/nicer, but do we have time to get to it?" types of issues. Maybe we can use this issue to sidle into it a bit...

stonea commented 1 year ago

Just to follow up from this after our module re-review meeting:

The current plan is to move these functions so they're methods on the BlockDist.Block and CyclicDist.Cyclic types and rename them to createDomain and createArray. Otherwise the interfaces will remain the same.

bradcray commented 1 year ago

@stonea: I think that https://github.com/chapel-lang/chapel/pull/21567 resolved this issue—do you agree?

stonea commented 1 year ago

Yes, this has been resolved, I'll close the issue.