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 RowVectorView type #1232

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tastaturtaste
Copy link

Add a view type for row vectors equivalent to the view type for column vectors. This should address issue #1229 with the explicit request for a PR. To make conversions from and to slices possible I had to change some lines of unsafe code in base/matrix_view.rs as described below. I think those changes should be fine, but please give them a thorough review.

Changes to unsafe code

To implement the From trait for conversion from and to slices for RowVectorView I had to adjust the implementation of IsContiguous for ViewStorage to avoid conflicting impls. The way I did this is to impose stronger trait bounds on the standing implementation to make room for the additional implementation for a 'ViewStorage' where the column stride is statically 1. Since the case where both the column and row stride are statically 1 would not be covered anymore with the additional trait bounds, I also added a implementation specifically for this special case.

@waywardmonkeys
Copy link
Contributor

If this moves forward, it would be nice if the docs were updated to be like what I did in #1266, assuming that also moves forward.

Add a view type for row vectors equivalent to the view type for column
vectors.
@Tastaturtaste
Copy link
Author

If this moves forward, it would be nice if the docs were updated to be like what I did in #1266, assuming that also moves forward.

I updated the doc comments accordingly...

Honestly, it's pretty demotivating making such a simple PR on explicit request (#1229 (comment)) and then getting no feedback at all.

@waywardmonkeys
Copy link
Contributor

@Andlon @sebcrozet It seems like this fell through the cracks ...

@Andlon
Copy link
Collaborator

Andlon commented Oct 3, 2023

If this moves forward, it would be nice if the docs were updated to be like what I did in #1266, assuming that also moves forward.

I updated the doc comments accordingly...

Honestly, it's pretty demotivating making such a simple PR on explicit request (#1229 (comment)) and then getting no feedback at all.

I'm sorry. Believe me, it's also very demotivating not to have time to review all the open PRs!

Alas, the breaking changes here require careful consideration, which I think is why I did not manage to "simply merge" this before. Unfortunately this is still the case for now.

I don't think nalgebra has From<&'a T> for View impls, and I'm not sure if we should add them. There's Matrix::from_slice for this purpose. I'm unsure why the current From<&Matrix> for &[T] is not sufficient, can you perhaps elaborate on why we need the new impls at all? (it's likely I missed something in passing)

@Tastaturtaste
Copy link
Author

I don't think nalgebra has From<&'a T> for View impls, and I'm not sure if we should add them. There's Matrix::from_slice for this purpose. I'm unsure why the current From<&Matrix> for &[T] is not sufficient, can you perhaps elaborate on why we need the new impls at all? (it's likely I missed something in passing)

I am not sure what you mean. There already was a DVectorView, which could also be converted from and to a slice. In my application, I, however, needed a row vector which didn't exist. So I implemented exactly the same interface as for DVectorView for the new type RowDVectorView. I did not add anything related to more general Matrix types at all.

Alas, the breaking changes here require careful consideration, which I think is why I did not manage to "simply merge" this before.

Which breaking change do you mean? There should be none.

@tpdickso
Copy link
Collaborator

I think the alleged breaking change might be in the adjustment to the unsafe trait impls for IsContiguous. However I think you are correct that it shouldn't be a breaking change to split one R impl out into two impls (IsNotStaticOne, U1), because as far as downstream users of the crate are concerned the trait should still be implemented for every type that previously implemented it.

I think you are also correct that the From<&Matrix> for &[T] impl is insufficient here, and that we need one for RowDVectorView for the same reason that the source also contains the corresponding specialized impls for DVectorView. (Perhaps because the blanket impl works with &DVectorView but not DVectorView?) So I think that this is good to merge, if we're okay with that.

@tpdickso
Copy link
Collaborator

@Andlon Is my understanding accurate that this isn't a breaking change? If so then I can merge this.

@tpdickso tpdickso requested a review from Andlon March 28, 2024 00:47
@Andlon
Copy link
Collaborator

Andlon commented Mar 28, 2024

@Andlon Is my understanding accurate that this isn't a breaking change? If so then I can merge this.

I believe it's a breaking change, because existing generic code might just have D: Dim bounds, but now it also needs D: IsNotStaticOne, which might not be possible to add in a generic context (since you might not be able to expect a generic caller to also supply this bound). It might still be the right thing to do, but it's definitely unfortunate...

@tpdickso
Copy link
Collaborator

@Andlon Is my understanding accurate that this isn't a breaking change? If so then I can merge this.

I believe it's a breaking change, because existing generic code might just have D: Dim bounds, but now it also needs D: IsNotStaticOne, which might not be possible to add in a generic context (since you might not be able to expect a generic caller to also supply this bound). It might still be the right thing to do, but it's definitely unfortunate...

Hmm, I think you're right. I can test it out. My thinking was that that shouldn't matter, because it's also implemented for the U1 case, so it shouldn't be necessary for downstream code to specify IsNotStaticOne on their code to still catch every type. But the compiler might not be able to realize that Dim is equivalent to Dim + IsNotStaticOne ∪ U1, particularly since Dim isn't a closed trait.

This is a problem with the lack of intersection/lattice impls, right? If the bound was just Dim then the impls for row and column slice views would overlap where both are U1. An unfortunate situation, considering there are no actual trait items so the overlap can't actually be problematic. 😆

@tpdickso
Copy link
Collaborator

Yup, you're right. Tragic!

use nalgebra::{Dim, IsContiguous, ViewStorage, U1};
fn first<'a, T, R: Dim, CStride: Dim>(storage: ViewStorage<'a, T, R, U1, U1, CStride>) {
    second(storage);
}
fn second<S: IsContiguous>(storage: S) {}
fn main() {}

Compiles against nalgebra but does not compile against this branch. I was going to say "presumably because it can't tell that when !IsNotStaticOne applies then another impl applies" -- but actually I'm not sure where I got the idea in the first place that there's another impl that should apply there. It seems like there is nothing on this branch that should apply to ViewStorage<'a, T, U1, U1, U1, Dim>. (Though... arguably there should be, right? A 1x1 view is always contiguous regardless of its stride. Not that it matters, because impl IsContiguous for ViewStorage<'a, T, U1, U1, U1, Dim> wouldn't solve the problem of not being able to supply Dim -- I tried it just now.)

A shame because this does seem like a great addition and an obvious hole in the API -- should we save this for a major version bump, then? The alternative could be to have a RowViewStorage which is effectively identical to ViewStorage, but which can implement a different set of traits. Really ugly... but maybe a necessary evil in a world where overlapping trait impls don't seem to be on the horizon any time in the foreseeable future? After all Dim + IsNotStaticOne isn't a correct trait bound anyway -- the user should be able to specify Dim, so it's not just a breaking change, it's one that would weaken the API.

@Tastaturtaste
Copy link
Author

Thank you for the consideration and review of this change. I did not foresee this breaking change, so I am glad you caught it. It is indeed very unfortunate.

[...] an obvious hole in the API

That was the motivation for this PR and the issue it stems from.

This is a problem with the lack of intersection/lattice impls

overlapping trait impls don't seem to be on the horizon any time in the foreseeable future?

Just a random thought, could this also be addressed using specialization? Not that that seems to happen anytime soon either...

As I am currently not using nalgebra anymore, I don't have the time to explore possible other resolutions to the issue. So please handle and change this PR however you see fit.

@Andlon
Copy link
Collaborator

Andlon commented Apr 3, 2024

I think I'd be fine with the breaking change. I think it makes the non-generic usage more symmetric/consistent. Generic code anyway doesn't have to rely on the Into implementation, as it can just use ::from_slice and ::as_slice instead, which anyway often is more appropriate.

So I think my vote is to accept the breaking change, and merge when we're ready for merging breaking changes.

@Andlon Andlon added breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes enhancement labels Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants