DM-13534: avoid usage of old ndarray PyConverter APIs #325
Conversation
Both old and new code depend on ndarray internals, but at least the new code depends on something that really ought to be public rather than something that has already been removed.
These are contiguous arrays, and we should tell NumPy that.
...in one file, to make sure it works. Removing the rest will be another ticket.
| CatalogT<Record> const& catalog, ///< Catalog | ||
| Key<T> const& key ///< Key to column to extract | ||
| ) { | ||
| ndarray::Array<typename Field<T>::Value, 1, 0> out = ndarray::allocate(catalog.size()); | ||
| ndarray::Array<typename Field<T>::Value, 1, 1> out = ndarray::allocate(catalog.size()); |
There was a problem hiding this comment.
How many more of these are out there? And would auto work nicely for them to prevent such issues in the future?
There was a problem hiding this comment.
These are not a big deal; it basically boils down to whether incrementing an iterator uses n += s (where s == 1) instead of just ++n. I just happened to notice it here.
auto won't help at all and in fact would hurt quite a bit, because ndarray::allocate doesn't return an Array and we're relying on implicit conversions in an expression-template kind of way.
| declareBaseColumnViewArrayOverloads<std::uint8_t>(cls); | ||
| declareBaseColumnViewArrayOverloads<std::uint16_t>(cls); | ||
| declareBaseColumnViewArrayOverloads<int>(cls); | ||
| declareBaseColumnViewArrayOverloads<float>(cls); | ||
| declareBaseColumnViewArrayOverloads<double>(cls); | ||
| // Angle requires custom wrappers, because ndarray doesn't | ||
| // recognize it natively; we just return a double view | ||
| // (e.g. radians). |
There was a problem hiding this comment.
This is breaking the encapsulation of Angle a bit right? Or does the API for getData specify that the values are in radians?
There was a problem hiding this comment.
Yes, it's breaking encapsulation, but it was broken before; I'm just breaking it differently now.
pybind11's own numpy APIs are now preferred, as that's what the ndarray pybind11 converters use.