Add helper for parsing tuples using nanobind #3025
Conversation
mgeplf
force-pushed
the
nanobind-parse-tuple
branch
from
4e5864d
to
72584bc
Compare
|
✔️ 72584bc -> Azure artifacts URL |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3025 +/- ##
==========================================
- Coverage 67.27% 67.27% -0.01%
==========================================
Files 572 573 +1
Lines 104944 104944
==========================================
- Hits 70605 70597 -8
- Misses 34339 34347 +8 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
src/nrnpython/utils.hpp
Outdated
| if (I >= t.size()) { | ||
| throw std::runtime_error("Not enough arguments"); |
There was a problem hiding this comment.
I think we should let t[I] do its Python thing. Upon out-of-bounds it will set the Python exception and return Null, which is likely what we want
https://docs.python.org/3/c-api/tuple.html#c.PyTuple_GetItem
There was a problem hiding this comment.
I think it's better if we are helpful with our error messages. I think that my current exception isn't worded well, and could be better, but that's a detail I need to fill in later.
More importantly, without the check, the code will segfault. Perhaps you have a different implementation in mind?
There was a problem hiding this comment.
So I guess something is confusing me. Why would t[i] segfault if it only calls PyTuple_GetItem under the hood?
There was a problem hiding this comment.
We now also have access to libfmt, so you can use that to improve the error message.
There was a problem hiding this comment.
I have updated the error message in the newest version.
src/nrnpython/utils.hpp
Outdated
| @@ -0,0 +1,31 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
The file can be called nrnpython/cast_tuple.hpp.
src/nrnpython/utils.hpp
Outdated
| } // namespace detail | ||
|
|
||
| template <typename... Ts, typename Tuple> | ||
| auto cast_tuple(Tuple&& tuple) { |
There was a problem hiding this comment.
Why any Tuple, if we know it's a const nanobind::tuple&?
There was a problem hiding this comment.
I was running into an issue trying to handle the nb::args case, and that's how I ended up working around it.
I have added a test if you want to try/suggest alternatives.
This comment has been minimized.
This comment has been minimized.
mgeplf
force-pushed
the
nanobind-parse-tuple
branch
from
2bfe2e2
to
8ab8a6c
Compare
* This can also be used for parsing `args`
mgeplf
force-pushed
the
nanobind-parse-tuple
branch
from
8ab8a6c
to
63e575a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
✔️ 63e575a -> Azure artifacts URL |
|
|
✔️ 738ff1c -> Azure artifacts URL |
|
@alkino / @1uc I seem to be getting errors with nanobind not being available: I'm confused as to why the tests are "automatically" including nanobind: My plan to deal with it is to only include the |
args