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

Optional support for nalgebra types #347

Merged
merged 10 commits into from
Sep 17, 2022
Merged

Optional support for nalgebra types #347

merged 10 commits into from
Sep 17, 2022

Conversation

adamreichold
Copy link
Member

@adamreichold adamreichold commented Sep 10, 2022

This is loosely based on https://github.com/qsib-cbie/nalgebra-numpy (which is based on https://github.com/fusion-engineering/nalgebra-numpy targetting an older version of pyo3) but I tried to achieve a deeper integration into our existing types and optimized the ToPyArray implementation to copy contiguous data using ptr::copy_nonoverlapping.

Closes #263

@adamreichold adamreichold marked this pull request as ready for review September 10, 2022 11:14
@davidhewitt
Copy link
Member

Agreed that having this as an optional feature makes sense for now. If users wanted to make the ndarray backend optional there can be a longer design discussion to figure out how to make it switchable.

For this PR, do you think the method names should have nalgebra in them? E.g. try_as_nalgebra_matrix() or a new type wrapper .nalgebra().try_as_matrix()?

@adamreichold
Copy link
Member Author

adamreichold commented Sep 11, 2022

For this PR, do you think the method names should have nalgebra in them? E.g. try_as_nalgebra_matrix() or a new type wrapper .nalgebra().try_as_matrix()?

I think this would be inconsistent with how we handle ndarray (as_array_view instead of the slightly stuttering as_ndarray_array_view) but this is not a particularly strong argument since we do handle both dependencies differently in any case.

I do think that the probability of confusion is small though as ndarray's "array" terminology is nicely distinct from nalgebra's "matrix" terminology.

One major issue I see is discoverability, e.g. searching for nalgebra will not yield those methods. But I think I would prefer to mitigate this using doc aliases:

grafik

I will will also extend the top-level documentation to give an overview of the nalgebra support as introduced here.

@adamreichold adamreichold force-pushed the nalgebra-support branch 2 times, most recently from 9fc9928 to 96d7a06 Compare September 11, 2022 09:24
Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thank you for this hard work!
I'm new to the nalgebra crate, but I enjoyed learning its unique features while reviewing 🙂
I have a few suggestions:

  • Note that the converted NumPy array from nalgebra has always fortran-layout
  • Conversion tests for a few more aliases (e.g., Vector3)
  • Maybe we might want to ask some original authors (e.g., @de-vri-es) for a quick review if it's OK? I

src/convert.rs Show resolved Hide resolved
src/array.rs Show resolved Hide resolved
@adamreichold
Copy link
Member Author

adamreichold commented Sep 14, 2022

* Note that the converted NumPy array from nalgebra has always fortran-layout

Will do.

* Conversion tests for a few more aliases (e.g., `Vector3`)

Will do.

(I do see room for future improvements here as we currently convert a Vector3 into a 3x1 array with dimensionality two instead of an array with dimensionality one. But I wanted to keep things simple for now.)

* Maybe we might want to ask some original authors (e.g., @de-vri-es) for a quick review if it's OK? I

Certainly. I think pinging them via your comment should be enough, right?

@adamreichold
Copy link
Member Author

Will do.

Done.

@de-vri-es
Copy link
Contributor

Honestly, it's been a while since I've looked at the numpy memory layout. I would trust someone who has dug through the docs more recently more than me ;)

@kngwyu
Copy link
Member

kngwyu commented Sep 16, 2022

I would trust someone who has dug through the docs more recently more than me ;)

Thanks. Fair enough 😅

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks :) just some comments and nits from me.

src/array.rs Show resolved Hide resolved
src/array.rs Outdated Show resolved Hide resolved
src/slice_container.rs Outdated Show resolved Hide resolved
@adamreichold adamreichold merged commit cde7906 into main Sep 17, 2022
@adamreichold adamreichold deleted the nalgebra-support branch September 17, 2022 13:30
@kngwyu
Copy link
Member

kngwyu commented Sep 18, 2022

Thank you!

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.

Support for nalgebra
5 participants