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

Enable reshape_generic for slices + other things #1180

Merged
merged 7 commits into from Jan 14, 2023

Conversation

Andlon
Copy link
Sponsor Collaborator

@Andlon Andlon commented Nov 24, 2022

This PR consists of 3 commits, which can be split out as different PRs as necessary:

  • Add U0, U1, ... constants alongside the U0, U1, ... types. This lets us write U4 instead of U4::name() or Const::<4> when we need const dimensions. It used to be like this before we got const generics, but it got lost in the transition. I don't think there's any implied breakage from this.
  • Rename Dynamic to Dyn and make Dyn a tuple struct.
    • This hasn't really been discussed much before, but it's a simple change so I thought I'd give it a go and see what people think.
    • This means we can write Dyn(3) instead of Dynamic::new(3), which is very verbose.
    • It also makes error messages like Matrix<T, _, Dynamic, VecStorage<...>> more concise, which might make them easier to read.
    • To avoid breakage, a type alias Dynamic = Dyn is also provided.
    • We might consider deprecating Dynamic.
  • Implement ReshapableStorage for matrix slices (only for unit strides at the moment), which was missing.
    • This means we can use slice.reshape_generic(...) on slices as well, something I've needed many times.
    • I discovered that there were no real tests for the existing reshape_generic functionality, so I added some.

This PR can either be merged before #1178, in which case I'd update #1178, or it can be merged afterwards, in which case I'll update this PR with the new view terminology.

@Andlon
Copy link
Sponsor Collaborator Author

Andlon commented Nov 24, 2022

CI failure seems to be due to a spurious failure of an eigen proptest.

@Andlon
Copy link
Sponsor Collaborator Author

Andlon commented Dec 19, 2022

Now that #1178 was merged, I will endeavor to update this PR soon. @sebcrozet: any thoughts on whether you would want to merge all 3 parts of this PR, or if I should leave some out (either for a separate PR or not at all, if the changes are not desirable).

@sebcrozet
Copy link
Member

sebcrozet commented Jan 1, 2023

@Andlon Thank you for this PR! Keeping the 3 parts of this PR is fine. I agree with deprecating Dynamic.

Andlon and others added 6 commits January 14, 2023 16:22
This allows us to simply write U4 in place of U4::name() or Const::<4>,
like we used to be able to before const generics.
Provide a type alias to avoid breaking code. Make Dyn a
tuple struct so that we can use the succinct syntax
Dyn(n) instead of Dyn::new(n).
In the process of implementing ReshapbleStorage for SliceStorage(Mut),
I discovered that there appears to be no tests for the existing
reshape_generic functionality on owned matrices.
@sebcrozet
Copy link
Member

@Andlon I took the liberty to update the PR and deprecate Dynamic in favor of Dyn. I’m merging this now.
Thank you again for this PR!

@sebcrozet sebcrozet merged commit f50b081 into dimforge:dev Jan 14, 2023
@Andlon
Copy link
Sponsor Collaborator Author

Andlon commented Jan 14, 2023

@sebcrozet: that's amazing, thank you! Pretty swamped with work and life at the moment, so haven't been able to dedicate any time to this. Great to see it merged and in the new release :D

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.

None yet

2 participants