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 .item() to Matrix1 #1231

Merged
merged 1 commit into from
Apr 30, 2023
Merged

Add .item() to Matrix1 #1231

merged 1 commit into from
Apr 30, 2023

Conversation

JulianKnodt
Copy link
Contributor

Allows for converting a Matrix1 to a scalar without having to index.

The test is implemented as a doctest, since I couldn't find anything else that would check whether or not code compiles.
Under the hood, it still just indexes, but the method is only available on Matrix1.

One question is whether or not this should return a borrowed value or an owned value. I assume borrowed, but for most cases it would likely be immediately dereferenced.

Closes #1209

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Apr 13, 2023

I think this is useful functionality that I could have needed myself many times, but I think the name .item() is not very descriptive. Personally I wouldn't know what this means.

What about .to_scalar(&self) -> T, .into_scalar(self), .as_scalar(&self) -> &T and .as_scalar_mut(&mut self) -> &mut T? It might seem excessive to have four methods, but there's a use case for all of these, and it seems the most consistent to provide all of them 🤔 Perhaps .to_scalar() is slightly redundant, although I think it would make sense to provide it.

@JulianKnodt
Copy link
Contributor Author

I'm fine with any name, but the reason I chose .item() was because that was what PyTorch calls their function that does the same thing. One thing I would note that is that it is written to be generic over types T, which may not technically be a "scalar" by the standard definition

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Apr 14, 2023

I see. Well, I think PyTorch is a very different kind of library than nalgebra, and there's already precedent in that almost every method in nalgebra already has the bound T: Scalar. Moreover, the use of scalar to denote an element in a vector is consistent with Wikipedia's article on scalars:

A quantity described by multiple scalars, such as having both direction and magnitude, is called a vector.[4] The term scalar is also sometimes used informally to mean a vector, matrix, tensor, or other, usually, "compound" value that is actually reduced to a single component. Thus, for example, the product of a 1 × n matrix and an n × 1 matrix, which is formally a 1 × 1 matrix, is often said to be a scalar.

While it's possible to store things that aren't strictly scalars in nalgebra data types, this is not nalgebra's main mode of operation. You'll almost always store integers, real or complex numbers, which all fit the definition of a scalar.

@JulianKnodt
Copy link
Contributor Author

makes sense, certainly while they are quite different, I sometimes just use PyTorch as a linear algebra library.
I'll update the method names soon!

@JulianKnodt
Copy link
Contributor Author

I didn't add .to_scalar(&self) -> T intentionally, I think that only works for Copy types, but if the type is Copy then it doesn't matter if it takes self by reference.

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Apr 17, 2023

@JulianKnodt: I think to_scalar can be implemented as self.as_scalar().clone(), which I think it would make sense to provide because I suspect this might be the most commonly used method of the four.

// be a null-ptr, as specified by the safety guarantees of `as_ptr()`.
// It would be nice to have this not require this though.
unsafe { core::ptr::read(self.as_ptr()) }
}
Copy link
Sponsor Collaborator

@Andlon Andlon Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is unsound. For example, take a MatrixView1 and call .into_scalar().

A safe way to implement this would be to instead implement it directly on Matrix1 and destructure the underlying array with let [ [ scalar ] ] = array.

Copy link
Contributor Author

@JulianKnodt JulianKnodt Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea how the underlying storage is laid out for MatrixView1. It somehow seems counterintuitive for me that the pointer would not point to the one element if it's a view.

Edit: Oh I see what you mean, if T doesn't implement Copy

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Apr 17, 2023

I haven't yet made a full review yet, but I just wanted to ask you to check your docs with cargo doc. I believe you're missing some newlines to properly divide the "one-line summary" and the full method documentation.

@JulianKnodt
Copy link
Contributor Author

I have no idea how to get access to the underlying array, but implementing it directly on Matrix1 (which uses owned storage?) is safe? Is it just self.data?

src/base/matrix.rs Outdated Show resolved Hide resolved
Allows for converting a `Matrix1` to a scalar without having to index.
@sebcrozet
Copy link
Member

Thanks @JulianKnodt for the changes and @Andlon for the review!

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.

Function that converts Matrix1 into just item?
3 participants