-
Notifications
You must be signed in to change notification settings - Fork 195
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
Separate const of ndarray from const of its data. #491
Conversation
Hmmm, macos C++ compiler doesn't like |
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 |
Thanks, Roman. I'm now accessing the class static data member
and, if
|
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? |
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.
Also, I was not sure what to do about
I'll be away for a few days, so no hurry responding. |
Since template parameter packs are recursively examined right to left, given 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. |
As a brainstorming exercise, I experimented some in PR #498 |
Add constexpr static bool is_ro to ndarray.
A static assertion ensures that the ``order`` and ``framework`` cannot be changed once set. For example, a template argument to ``view()`` can only change one of these if it has not been set. The scalar element type and the ``shape`` can be set multiple times, and the rightmost one takes effect. Thus, these can be changed by specifying template arguments to ``view()``. The read-only boolean, ``is_ro``, can be set either by specifying ``nb::ro`` or by specifying a const-qualified scalar element type. It is sticky. Once set, it cannot be cleared. So, a read-only view of writable data can be obtained, but not the reverse.
I re-based this upon the current HEAD (so as to manually resolve merge conflicts with recent commits) and force-pushed. |
d7117a4
to
983d6c0
Compare
@@ -698,14 +709,6 @@ section <ndarrays>`. | |||
``shape()``, ``stride()``, and ``operator()`` following the conventions | |||
of the `ndarray` type. | |||
|
|||
.. cpp:function:: template <typename... Ts> auto& operator()(Ts... indices) |
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.
What's the reason for this removal? AFAIK this operator is still there, and it should be documented.
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.
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()
.
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.
Or, lines 807-814 if you're looking at diff after the merge commit....
Sorry for leaving this dormant for a long time. I mainly just have two pieces of feedback. |
Thank you! |
A static assertion ensures that the ``order`` and ``framework`` cannot be changed once set. For example, a template argument to ``view()`` can only change one of these if it has not been set. The scalar element type and the ``shape`` can be set multiple times, and the rightmost one takes effect. Thus, these can be changed by specifying template arguments to ``view()``. The read-only boolean, ``is_ro``, can be set either by specifying ``nb::ro`` or by specifying a const-qualified scalar element type. It is sticky. Once set, it cannot be cleared. So, a read-only view of writable data can be obtained, but not the reverse.
Create a SID with const element type if a Nanobind array is defined read-only. Requires Nanobind 2.1.0 (with merged wjakob/nanobind#491). --------- Co-authored-by: Ioannis Magkanaris <ioannis.magkanaris@cscs.ch>
Create a SID with const element type if a Nanobind array is defined read-only. Requires Nanobind 2.1.0 (with merged wjakob/nanobind#491). --------- Co-authored-by: Ioannis Magkanaris <ioannis.magkanaris@cscs.ch>
My understanding is that the following should succeed because
a
references a writable array, and it does not matter thata
itself is const qualified. I have immutable metadata, but the data of the actual array is mutable.However, this does not compile because
a.data()
returns a pointer-to-const:Also, the following compiles, but it should not because the
nb::ro
should imply thata.data()
returns a pointer-to-const: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.