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

Add generic initializers for statically-dimensioned array types #1385

Conversation

thatcomputerguy0101
Copy link

@thatcomputerguy0101 thatcomputerguy0101 commented May 11, 2024

This takes advantage of const generics to provide general initializers for the statically-dimensioned array types. (2-6 dimensions, with the current implementations). This reduces the reliance on macros for array initialization, and enables other crates to operate more succinctly when working with an unknown-dimension static array type. I'm unsure how well this fits with the existence of the dynamic array index implementation, and the name of the new type alias IxD is open for discussion. No additional test cases have been implemented for this functionality yet.

@bluss
Copy link
Member

bluss commented May 12, 2024

Hm, new clippy lints should be fixed in a different PR, I think

@thatcomputerguy0101
Copy link
Author

I originally decided on the name IxD to match the style of Ix0, Ix1, etc..., but with a generic D instead of a fixed value. It is also short, which helps in its usability as a convenience type (since it is an alias for Dim<[Ix, D]>). However, its name is close to IxDyn, and I have concerns about possible confusion between those two.

@thatcomputerguy0101
Copy link
Author

With my most recent updates, I believe all static-related simplification that can be done within the current Rust feature set is implemented. This can be revisited in the future once rust-lang/rust#76560 lands.

@thatcomputerguy0101 thatcomputerguy0101 marked this pull request as ready for review July 6, 2024 14:31
@thatcomputerguy0101
Copy link
Author

I added some basic tests for the new initializers. The other changes should all be covered by the existing tests since they were just implementation refactorings.

@thatcomputerguy0101
Copy link
Author

I was having some problems with getting rust-analyzer in VS Code on my system to correctly see the new test file, but it seems to be picked up by command line clippy and cargo test.

@bluss
Copy link
Member

bluss commented Aug 2, 2024

I'm sorry about the delay here.

I now understand that this PR adds IxD (quibble: too similar to the existing name IxDyn).
IxD<2> has the same job as Ix2, only the first is const generic.

We can't accept this change at the moment, we don't want an aliasing dimensionality - two different ways to create a (statically known) two dimensional ndarray.

However, of course we need to use const generics better, and incremental changes is the way to get there, but I can't see us taking this path.

Something fruitful would be removing Ix2 and replacing with a better implementation, while preserving backwards compat with existing code as much as possible. (Introduce new type alias Ix2 and function Ix2?).

@bluss bluss closed this Aug 2, 2024
@thatcomputerguy0101
Copy link
Author

Yeah, I had concerns about the name too. This is not adding a distinct type, only an additional alias, so Ix2 and IxD<2> are already the same type. This just relates all of the static array types under the IxD name (and adds the repeating factory). Technically, most of these changes could instead be implemented directly on Dim<[Ix; D]>, although that is less discoverable than giving it a dedicated name.

@thatcomputerguy0101
Copy link
Author

thatcomputerguy0101 commented Sep 2, 2024

I suppose underlying the exact methodology, I am seeking a way to access arbitrary diagonal elements of an unknown statically-dimensioned ndarray. I was doing this with a combination of IxD::repeating(i: Ix) and IxD::new(ix: [Ix; D]), but the same effect could also be achieved if indexes had a map function similar to the built-in static arrays. Would this be a change you are more open to accepting?

@thatcomputerguy0101
Copy link
Author

Actually, I missed that basic arrays already implement IntoDimension, which might suit my needs.

@thatcomputerguy0101
Copy link
Author

Seems like that does work; I think I tried that before, but was missing a trait constraint in my generalized code.

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

Successfully merging this pull request may close these issues.

2 participants