-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 Eigen::Tensor
& Eigen::TensorMap
support
#4201
Conversation
Skimming this, the casters should probably be refactored to using the EigenProps helper struct? Unless there is a reason it's defining it's own helper structs? |
@Skylion007 I'm hesitant to merge the two since there wouldn't be that much shared code. Most of the EigenProps helper construct is dedicated to dealing with weird setups with partially dynamic matrix sizes, which Tensor doesn't do. |
@Skylion007 Can you do me a favor and trigger another run of the CI? I think I fixed the build errors on older versions of C++. |
@lalaland Perhaps the common code snippits between the two should be declared in another class/struct that they inherit from then? |
@Skylion007 It turns out I wasn't even used the shared logic (which was just a mutable_data vs data call on array), so I deleted it. There is now currently no overlap between the two. |
@Skylion007 Thanks for the feedback! I think I addressed all of the issues you pointed out. |
Very quick high-level impression after looking for only a couple minutes: it would be much better IMO to add this code under a separate eigen_tensor.h. Additional though with less certainty: include eigen_tensor.h from eigen.h, or maybe do a more complete job, move the current implementation to eigen_matrix.h (if that name makes sense), and eigen.h includes both. Background thought: in this PR it's obvious what's old (not tensor) and what's new (tensor), but once it's merged it's one big soup for everybody else. |
@rwgk Good and easy suggestion. Done. |
3d8b19e
to
c034034
Compare
Something quick & trivial: to avoid the auto fixes, run The first time through it can take a couple minutes, after that it's less than 12 seconds on my machine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more nits.
} | ||
|
||
handle result = array_t<typename Type::Scalar, compute_array_flag_from_tensor<Type>()>( | ||
H::get_shape(*src), src->data(), parent_object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent_object gets converted to handle here. If it's not released, it will decrefed when it goes out of scope, which I think is slightly different behavior from what you had before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a release right there in the next line? Anyways, I swapped these to release at the return statement instead.
@rwgk Wasn't able to get that to work on my machine. I'll just squash this PR once all comments are resolved (so they won't clutter the git history). |
It will be auto-squashed when we merge. Please don't squash manually, that'd make it more difficult for reviewers to follow, and hard for reviewers and you to backtrack. BTW: I may only get a chance to look carefully this weekend. |
Does anyone have any tips for setting up an environment to debug those windows pypy errors? |
No that's hard.
We have a ton of pytest skips already for PyPy.
Keep the PyPy failures for last.
If everything else works except PyPy my vote is: skip
…On Fri, Sep 30, 2022 at 16:05 Lalaland ***@***.***> wrote:
Does anyone have any tips for setting up an environment to debug those
windows pypy errors?
—
Reply to this email directly, view it on GitHub
<#4201 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFUZABDSMJ35OEKIAPTSYDWA5W2NANCNFSM6AAAAAAQXCSELM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
A quick thing (probably) I could do is run Google internally with clang
sanitizers. (Very easy internally; I don't know how to do it externally
although I believe it's possible in theory.) Is this PR at a stage where it
would make sense for me to try? Are the tests reasonably complete already?
…On Fri, Sep 30, 2022 at 16:11 Ralf Grosse-Kunstleve ***@***.***> wrote:
No that's hard.
We have a ton of pytest skips already for PyPy.
Keep the PyPy failures for last.
If everything else works except PyPy my vote is: skip
On Fri, Sep 30, 2022 at 16:05 Lalaland ***@***.***> wrote:
> Does anyone have any tips for setting up an environment to debug those
> windows pypy errors?
>
> —
> Reply to this email directly, view it on GitHub
> <#4201 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFUZABDSMJ35OEKIAPTSYDWA5W2NANCNFSM6AAAAAAQXCSELM>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
@rwgk That might be a good idea. I just added the remaining tests for all the current expected behavior. Although I'm not sure that clang's sanitizers will catch anything. The code is already clean according to valgrind. |
Cool, I'll try, thanks! I don't have exact data, but from memory, we had cases where one or the other (valgrind, clang) didn't catch something, either way around. |
I had some fun resolving smart_holder merge conflicts, and hiccups with my usual Linux build environment (I'm using scons), but I got those resolved, too. But when trying to build with our internal toolchain I'm running into errors that seem related to the Eigen version. In my interactive standard Linux build the Eigen version is 3.4.0, but with our internal toolchain it is 3.4.90. That version was installed from this tar file: (This is the first time that I'm having issues with our internally used Eigen version not working while the pybind11 GitHub CI and my interactive standard Linux are fine.) I'm pasting the error message below. I haven't tried to understand it yet, hoping you may understand it quicker because you are probably much more familiar with the Eigen code.
|
Is there anything left here to do? I put in a few hours of my time and REALLY would like to get this merged as soon as possible, so that I get a return on the investment. I'm sure this will get picked up quickly. |
As far as I know, I think I fixed everyone's comments and all tests are passing (except for a flake that someone should rerun). I also need to update the docs, but I would prefer to do that in a separate PR. |
@rwgk @henryiii Were we not planning to put into the next major version release? We need an upgrade guide as this could break people's code that define their own Eigen::Tensor casters. Something to the effect of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from the few nits I pointed out.
This is a really good point. I'll modify this PR so that pybind11/eigen.h only includes eigen/matrix.h. This way we can guarantee no breakages of existing code. |
Thoughts on this @rwgk @henryiii? I could go either way with having eigen.h define the eigen/matrix.h or not. We could also gradually change behavior eigen.h at the next major release, but just add in the refactor the new include file for now. |
I'm not sure. But what comes to mind: doing something special usually does both good & harm. Until now I was thinking of eigen.h as a convenient way to include both eigen/matrix.h and eigen/tensor.h. But now that that we have this discussion: is that even a good idea / what we want to promote? IWYU seems far better in general. That brings me to thinking of eigen.h as a header deprecated in 2.11 and removed in (e.g.) 2.13. Which means we will/should never include eigen/tensor.h from eigen.h, now or in the future. |
Awesome, thanks everybody who commented here! I looked at the CI failures: Centos 7 has been broken for a week already, the other failure is a github infrastructure hiccup (download failed). Good to ignore I'm sure. |
Eigen::Tensor
& Eigen::TensorMap
support
@lalaland while integrating this PR into the smart_holder branch I noticed this block (somehow I missed it before): pybind11/tests/test_eigen_tensor.py Lines 8 to 13 in fab1eeb
If something goes wrong (refactoring, new platform, etc.) it could easily go unnoticed that something is wrong. Is it ever expected that the import fails, given that the |
eigen_tensor and eigen_tensor_avoid_stl_array are from two separate C++ files, so eigen_tensor_avoid_stl_array will sometimes be unimportable if that C++ file never got built. You do get a warning from pybind11/tests/test_eigen_tensor.py Line 37 in fab1eeb
Do you want me to upgrade pybind11/tests/test_eigen_tensor.py Line 37 in fab1eeb
|
In what situation(s) is that expected? |
When you use DPYBIND11_TEST_OVERRIDE to only build a subset of the tests. |
Ah, hm ... I feel we could make it a hard requirement that the two always have to be built together, then have a plain Alternatively, a very outstanding WARNING in the Something else minor I noticed: pybind11/include/pybind11/eigen/matrix.h Line 14 in fab1eeb
That line seems to be outdated, I don't see the comment copied into tensor.h anymore (good; one place is enough). Could you please remove that line in matrix.h? |
I sent #4255, implementing what I suggested above. |
Description
Adds Eigen::Tensor support to eigen.h.
Eigen::Tensor allows you to handle np.arrays with more than two dimensions, which is quite handy in many cases.
I also added tests for all the relevant behavior.
Note that the Eigen::TensorMap implementation could be better if we allow std::optional, but I'm not sure if we are allowed to require that in this codebase.
Closes #1377
Closes #2119
Suggested changelog entry: