Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[de-feature request] tuples for const-defined dimensions #622

Open
swfsql opened this issue Apr 24, 2019 · 7 comments · May be fixed by #627
Open

[de-feature request] tuples for const-defined dimensions #622

swfsql opened this issue Apr 24, 2019 · 7 comments · May be fixed by #627

Comments

@swfsql
Copy link

swfsql commented Apr 24, 2019

updated to #622 (comment)
(so tuples replacement for arrays, and also still using arrays for 0-sized and 1-sized dimensions)


Hello, I'm new to this ecosystem here- and thanks a lot for your work on it!
As I was using the ndarray crate - it may be just me but - I thought that "what if we always use a tuple to indicate a shape's dimension?".

I mean, this is already how it works most of the time, except for the Ix1 and the dynamic case.
That is, the only case, for compile-time defined number of dimensions, that doesn't fall into what I said above was the Ix1 case, where a single usize is used as a Pattern to indicate a 1-dimensional array.

I propose a de-featuring (feature removal) so that Ix1 case would also require a 1-sized tuple, similarly to the Ix2 (Ix3, etc) cases.

This would simply increase verbosity whenever a Ix1 were to be used, so.. well.. maybe this [proposal] would be taking a step into the wrong direction.
(I'll be PRing for a reference since this would require simple changes after all).

@jturner314
Copy link
Member

Hi @swfsql, and welcome to the ecosystem!

Will you please provide more information about the motivation for doing this? I generally find it convenient to use 5 instead of (5,) and haven't experienced any problems with this. If you want to use (usize,) as the type instead of usize, that's supported too.

@swfsql
Copy link
Author

swfsql commented Apr 25, 2019

Thanks @jturner314 ,
I initially got motivated when I called .dim() I expected a (usize,) return type - as an inexperienced user - since I was used to understand tuple length as dimension quantity after using the crate for a while.

This specific change - the return type of dim() - doesn't implies the additional change about building dimension information - about using usize or (usize,) for dimension construction - but I thought that for symmetry, if one change happened, the other should follow.

I hope this helps clarifying - and I'd like to restate that this is a pretty subjective ergonomic opinion from an inexperienced user! (and this change would surely be not welcomed for most users I believe)

@jturner314
Copy link
Member

Oh, okay, you're talking about Ix1::Pattern. I misunderstood. (I should have read the PR.)

Now that Rust supports destructuring fixed-size arrays in addition to tuples, I actually think we should change all the Dimension::Pattern types for const-dimensions from tuples to fixed-size arrays, so we'd have

  • Ix0::Pattern = [usize; 0]
  • Ix1::Pattern = [usize; 1]
  • Ix2::Pattern = [usize; 2]
  • ...
  • Ix6::Pattern = [usize; 6]
  • IxDyn::Pattern = IxDyn

Length-one arrays are syntactically nicer than one-element tuples because they don't need the extra trailing comma. Additionally, arrays are more convenient than tuples because they can be iterated over easily.

@swfsql, @LukeMathWalker, @bluss Thoughts?

@swfsql swfsql linked a pull request May 1, 2019 that will close this issue
@swfsql
Copy link
Author

swfsql commented May 1, 2019

@jturner314 I've submitted a PR for reference.
The changes were pretty straightforward and I personally think it's user-friendly to always use arrays for constant dimensions. But it still required a lot of changes, so this still would be a drastic change, api-wise.

@LukeMathWalker
Copy link
Member

LukeMathWalker commented May 1, 2019

Sorry for the delay in answering, but it's quite an intense period.
The change itself looks pretty good to me.
On the other hand though, are we gaining enough from this change considering its breaking one of the core interfaces?

I know we have plans to review the way the whole dimension story works, and those plans will probably have to take into account the new language features that we could use to make the API nicer (e.g. const generics and GAT, for this specific case). Should we repeatedly iterate on this section of the API or should we try to batch improvements together?

@swfsql
Copy link
Author

swfsql commented May 1, 2019

@LukeMathWalker I personally would enjoy the change but I agree, I don't it's worth for other users.
And for everyone, surely, it wound't worth to have a drastic API change that is destined to be changed again.

On the other hand, I think there is a chance that the interface (exclusively) pushed by const generics would likely be this very change (tuples -> fixed-sized arrays). But a more drastic re-design may change it completely, so there would really be no-gains for this change.

@jturner314
Copy link
Member

I think that we will end up switching from tuples to fixed-size arrays anyway when we overhaul the Dimension types. However, @LukeMathWalker makes a good point that since this change isn't urgent, we should wait to make it at the same time as all the other breaking changes to the Dimension types.

So, let's postpone this until we overhaul the Dimension types. That is currently blocked on const generics, specialization, and generic associated types. Fortunately, the Rust roadmap includes all of those features for this year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants