support None for const char* arg, with minimal overhead #683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since pybind11 also accepts None for const char* as an exception among built-in types, remove a paragraph about differences in porting.rst.
wjakob
force-pushed
the
master
branch
2 times, most recently
from
f9e5e0b to
30e96b7
Compare
wojdyr
added a commit
to project-gemmi/gemmi
that referenced
this pull request
Sep 13, 2024
Please report problems when you find them. notes and changes ----------------- Protocol Buffer is currently not supported by nanobind. Adding GridBase.__array__() for NumPy interoperability in many cases works as seemless replacement of the Protocol Buffer. Implicit list->ndarray conversion is also not supported, but is welcomed: wjakob/nanobind#327 None is not accepted by default, an annotation must be added. I may have missed it in some functions - let me know. In case of const char* arg, None is not supported at all; I proposed it upstream: wjakob/nanobind#683 For default nullptr, using nb::arg()=nb::none() instead of =nullptr. Both work, but the latter seems to be more canonical in nanobind. In nanobind 2.x using make_iterator and bind_vector+__getitem__ returns copies, not references, by default. We usually prefer to avoid copying. Added a helper function usual_iterator() that uses old rv_policy. gemmi.ValueSigmaAsuData.value_array: previously had so-called structured data type, now it has shape (N,2) with dtype=float32. Added pickling support for Structure, Model, Chain, Residue, Atom, UnitCell. Notes about pickling of SpaceGroup: wjakob/nanobind#670 In a follow up, picking of SpaceGroup was optimized, replacing xhm() with an index. The gain was modest (pickling and unpickling a list of all SpaceGroups from the built-in table was reduced from ~1ms, to ~0.5ms). It might not be safe - multiple copies of the table may exist. Notes about constructors for PdbWriteOptions and MmcifOutputGroups: see wjakob/nanobind#664 doctests: many changes, mostly because __repr__ of enums and of bind_vector-ed classes in nanobind is different than in pybind11 added alignas(8) to HklValue: in nanobind array bindings, stride is given as a number of elements, not bytes. This was a problem for ComplexAsuData.value_array: sizeof(HklValue) was 20 (3*4+2*4) which is not a multiple of 8. Functions IT92Coef.calculate_sf() and IT92Coef.calculate_density_iso() used to be vectorized (could take numpy array as arg), now they aren't, let me know if you need a vectorized version. Not yet done: *.pyi files with type annotations Planned: unified logging of warnings/errors from various gemmi functions
wjakob
force-pushed
the
master
branch
2 times, most recently
from
f3e2796 to
bff96e2
Compare
hpkfft
reviewed
Dec 21, 2024
| @@ -286,7 +286,8 @@ template <> struct type_caster<char> { | |||
| value = PyUnicode_AsUTF8AndSize(src.ptr(), &size); | |||
| if (!value) { | |||
| PyErr_Clear(); | |||
| return false; | |||
| // optimize for src being a string, check for None afterwards | |||
| return src.is_none(); | |||
There was a problem hiding this comment.
Maybe you want:
return src.is_none() && flags & (uint8_t) cast_flags::accepts_none
where flags is the name given to the second argument to from_python() above.
But I'm not the expert on such things....
There was a problem hiding this comment.
I think it's not needed. accepts_none is checked in nb_func_vectorcall_complex() and None can't go through nb_func_vectorcall_simple(). I remember testing it, but I could be missing something.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since pybind11 also accepts None for
const char*as an exception among built-in types, remove a paragraph about differences in porting.rst.The overhead is minimal. Checking the size of
test_functions_ext.cpython-311-x86_64-linux-gnu.sobuilt with -O3, it increased only by 32 bytes.I'm aware that it's possible to use
std::optional<const char*>, but to me it looks more awkward thanstd::optional<int>, perhaps becauseconst char*, unlikestd::string, looks already optional.There is a third place in docs where None arg is covered:
nanobind/docs/functions.rst
Lines 178 to 181 in 8e5fd14
Perhaps it's outdated, because having argument such as
int*works fine, just doesn't accept None. Or I misunderstand it. Anyway, I didn't try to edit it.