added from_cpp() method for the Eigen::Ref type_caster

[ThoreWietzke]

As discussed in issue #330, Eigen::Ref was missing the from_cpp() method in it's type caster. I essentially copied the from_cpp() method from the Eigen::Map type caster.

[wjakob]

@WKarel, could I ask you to take a look to see whether this change makes sense as-is, or whether there are dangers involved in passing references around like this?
(I know very little about the Eigen map/ref types.)

[WKarel]

type_caster<Eigen::Map<...>>::from_cpp yields a numpy.ndarray that

• does not own the data it refers to, and that
• does not otherwise keep alive that data (even though it has a .base object, but that is a buffer provider who does not keep alive that data, either).

So this caster does not ensure that the returned object stays valid (it may in fact be invalid immediately), and passing an Eigen::Map to Python is inherently unsafe. But when using Eigen types, this may be the only way to efficiently call into Python for requesting changes (without copying the data forth and back). So I think that this is important to support. Since an Eigen::Map obviously does not own its data, the risk is at least obvious on the C++ side.

I think that one may similarly see handing over Eigen::Ref to Python. The difference is that an Eigen::Ref may or may not own the data it maps, while an Eigen::Map never does.

As clarified already in #330, the use case already works when using Eigen::Map instead of Èigen::Ref. The benefit I see in supporting the cast of Eigen::Ref to Python is that unlike Eigen::Map, Eigen::Ref offers conversion constructors from other Eigen types, making Eigen::Ref more convenient to work with.

Apart from that, I would re-use type_caster<Eigen::Ref<...>>::MapCaster::from_cpp as far as possible, instead of copy/pasting the code from type_caster<Eigen::Map<...>>.

How about some test cases?

[wjakob]

Thank you @WKarel 🙇

[ThoreWietzke]

Thank you for your remarks @WKarel. The test cases would be similar to #331, which exclusively tests the input of Eigen::Map. I can provide them.

[WKarel]

Additionally, I would at least remove the trailing return type declarations from castToRefVXi, castToRefCnstVXi, castToDRefCnstVXi, and castToRef03CnstVXi in test_eigen.cpp.

[ThoreWietzke]

Additionally, I would at least remove the trailing return type declarations from castToRefVXi, castToRefCnstVXi, castToDRefCnstVXi, and castToRef03CnstVXi in test_eigen.cpp.

When I remove the type declarations, the castToRefCnstVXi fails, as the current implementation of from_cpp fails to properly cast the underlying data. With the declarations, the base type caster is called which correctly maps the data.

Another question, reusing the MapCaster::from_cpp() method fails for the Eigen::Ref type caster, as it's specificly written for Eigen::Map. So what did you meant with reusing @WKarel ? Maybe define from_cpp() for Eigen::Ref and reuse it inside the Eigen::Map caster?

[WKarel]

When I remove the type declarations, the castToRefCnstVXi fails, as the current implementation of from_cpp fails to properly cast the underlying data.

I suggested to remove these return type declarations as examples of how to test the new functionality, without thinking too much about it. Of course, if a np.ndarray of non-matching type is casted into a Ref<const ...>, then it owns the data it maps. And since it is a temporary in castToRefCnstVXi, the returned np.ndarray refers to memory that has been destroyed upon exit from that function. So I suggest you find other ways for testing.

Concerning re-using MapCaster::from_cpp(), my idea was to create a temporary Map from the Ref in type_caster<Eigen::Ref<...>>::from_cpp, and pass that to type_caster<Eigen::Map<...>>::from_cpp. However, having had a closer look into it, creating a Map from a Ref is quite elaborate, again. Given that, you could move the code of type_caster<Eigen::Map<...>>::from_cpp into a stand-alone template function that works for both Map and Ref. Or maybe it's okay with @wjakob to just copy/paste the code.

[wjakob]

A small amount of duplication is fine for me here.

[ThoreWietzke]

@wjakob then everything should be ready for your review

[wjakob]

Merged, thanks!