Separate const of ndarray from const of its data.

[hpkfft]

My understanding is that the following should succeed because a references a writable array, and it does not matter that a itself is const qualified. I have immutable metadata, but the data of the actual array is mutable.

    m.def("write_should_succeed",
          [](const nb::ndarray<>& a) {
              if (a.dtype() == nb::dtype<double>()) {
                  auto ptr = static_cast<double*>(a.data());
                  *ptr = 7.0;
              }
          });

However, this does not compile because a.data() returns a pointer-to-const:

error: static_cast from 'const Scalar *' (aka 'const void *') to 'double *' casts away qualifiers
                  auto ptr = static_cast<double*>(a.data());
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Also, the following compiles, but it should not because the nb::ro should imply that a.data() returns a pointer-to-const:

    m.def("should_not_compile",
          [](nb::ndarray<nb::ro>&& a) {
              if (a.dtype() == nb::dtype<double>()) {
                  auto ptr = static_cast<double*>(a.data());
                  *ptr = 7.0;
              }
          });

If you agree, please consider this PR.

Also, this adds the constexpr static bool is_ro to ndarray.
I guess I just thought that would be nice....
If you prefer another name, or don't like it at all, that's fine too.

[hpkfft]

Hmmm, macos C++ compiler doesn't like static_assert(a.is_ro); in the tests.
Is there a mac-specific idiom that should be used?
If not, I can simply && this into the return value and let the python assert check it.

[rzhikharevich]

Hmmm, macos C++ compiler doesn't like static_assert(a.is_ro); in the tests. Is there a mac-specific idiom that should be used? If not, I can simply && this into the return value and let the python assert check it.

The problem is that you are accessing a constexpr static member through an instance reference that is not constexpr. See this discussion. As I understand, this is only valid since C++23 due to P2280R4

[hpkfft]

Thanks, Roman. I'm now accessing the class static data member is_ro through the type of a

          [](nb::ndarray<nb::ro> a) {
              static_assert(decltype(a)::is_ro);

and, if a is a reference

          [](const nb::ndarray<nb::ro>& a) {
              static_assert(std::remove_reference_t<decltype(a)>::is_ro);

[wjakob]

In general, I am open to this change (modulo naming tweaks). However, I am concerned a bit by the magnitude and complexity of the added tests. Is it really necessary to add that much replicated code to test this change? Could you create a single test function that is simply instantiated multiple times?

[hpkfft]

Certainly. (I've had project managers that insisted on simple, repetitive, fall-through code in tests.)

Please advise (at your convenience) on naming preference, or if the boolean should be removed.
I was thinking it's useful for code like this:

template<typename... Ts>
std::ostream& operator<<(std::ostream& os, const nb::ndarray<Ts...>& a) {
    return os << "Array (" << ((a.is_ro) ? "read-only" : "writable")
              << ") at " << a.data();
}

Also, I was not sure what to do about auto v = a.view<double, nb::ndim<1>>(); if a is nb::ndarray<nb::ro>
Should the non-constness of the double override the read-only of a, or should the constness be "sticky"?
I probably won't be using views, so I don't have strong opinion.
I guess I'd probably go with sticky as more in line with the light-weight philosophy of views. In other words, the user probably intended const double. It's still possible to change a read-only ndarray to writable using

nb::ndarray<double> wa{a};  // wa is writable

I'll be away for a few days, so no hurry responding.

[hpkfft]

Since template parameter packs are recursively examined right to left, given nb::ndarray<float, nb::shape<2, 2>> a, the view a.view<nb::shape<4>>() is still 2x2. This patch changes this so that the shape for the view becomes 4. The implementation is that if the shape has already been set, ignore any more shapes that are encountered to its left.
I made it an assertion error to have two conflicting order or framework. Note that changing from 'C' to 'F' doesn't transpose the 2D array because it doesn't change the strides. So I thought it should be an error.
I made is_ro sticky. So ndarray<const float> and ndarray<nb::ro, float> and ndarray<float, nb::ro> are all the same.
In particular, a read-only view of writable data can be obtained but not the reverse.

It would be easy to change the code if another choice is preferred. The hard part is deciding on the best API from a user's perspective.

[hpkfft]

As a brainstorming exercise, I experimented some in PR #498

[hpkfft]

I re-based this upon the current HEAD (so as to manually resolve merge conflicts with recent commits) and force-pushed.

[wjakob]

What's the reason for this removal? AFAIK this operator is still there, and it should be documented.

[hpkfft]

It's not removed; it's on lines 692-696. The diff algorithm got confused (from a human perspective) and spliced things together weirdly.
There had been two data() functions documented, with and without const-qualified this. Now there is only one as the return value is const-qualified based on the writability of the actual data and not on the const qualification of the ndarray object itself.

I did move the documentation of operator() above that of view(). I thought it would be helpful for readers to see operator() next to data() so they could see both and decide what better suits their purpose. Also, view() documents that it provides operator() following the same conventions as ndarray, so it seemed nice to document all of ndarray's functions before discussing view().

[hpkfft]

Or, lines 807-814 if you're looking at diff after the merge commit....

[wjakob]

Sorry for leaving this dormant for a long time. I mainly just have two pieces of feedback.

[wjakob]

Thank you!