Skip to content

Render NaN default argument as numpy.nan #2243

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

Closed
wants to merge 4 commits into from

Conversation

sizmailov
Copy link
Contributor

@sizmailov sizmailov commented Jun 5, 2020

This PR changes how nan values are rendered in docstring.

this PR master
foo(x: float = numpy.nan) -> None
foo(x: float = nan) -> None

Implemented via float overloads for py::arg_v. Might be considered as an overkill for such a small change. On the other hand nan values are quite common sentinel values so it might be worth it to generate more toolable docstrings.

compare.cpp
#include "pybind11/pybind11.h"
#include "pybind11/embed.h"

namespace py = pybind11;

PYBIND11_EMBEDDED_MODULE(example, m) {
  m.def("foo", [](float x) {}, py::arg("x")=std::numeric_limits<float>::quiet_NaN()); 
}

int main() {
  py::scoped_interpreter guard{};
  py::exec(R"(
      from example import * 
      print(foo.__doc__, end="") 
)");
}

sizmailov added 2 commits June 5, 2020 19:33
Example: generated signature changes from
    foo(x: float = nan)
to
    foo(x: float = numpy.nan)
std::isnan is not available on gcc-4.8
@sizmailov sizmailov force-pushed the doc-gen-nan-default-arg branch from 9136bac to a30af8f Compare June 6, 2020 07:53
@sizmailov
Copy link
Contributor Author

sizmailov commented Jun 6, 2020

__cplusplus >= 201103L doesn't mean the library is c++-11 complaint in gcc-4.8 (std::isnan is missing).
To avoid mess with other compilers versions I've enabled it starting from c++14.

@sizmailov sizmailov force-pushed the doc-gen-nan-default-arg branch from 54fd417 to d4784d1 Compare June 6, 2020 08:22
@sizmailov
Copy link
Contributor Author

On second thought I think I chose wrong place inject the change.

The underlying bigger problem is __repr__ function is not intended to produce valid python expressions, therefore we get malformed function signatures quite often. Probably next suggestion is better candidates to format value for signatures.

if (a.descr)
a.descr = strdup(a.descr);
else if (a.value)
a.descr = strdup(a.value.attr("__repr__")().cast<std::string>().c_str());

For example new extension point can be introduced with something like

tempate<typename T>
struct value_formatter {
   value_formatter(const T& value);
   const char* operator() const;
}

// and later in pybind11.h

if (a.descr)
    a.descr = strdup(a.descr);
else if (a.value)
    a.descr = strdup(value_formatter(cast(a.value))());

@sizmailov sizmailov marked this pull request as draft June 10, 2020 01:48
@wjakob
Copy link
Member

wjakob commented Jun 10, 2020

I'm curious: what's wrong with just nan? I don't think numpy.nan is a good alternative, pybind11 is used in all sorts of places that don't even have numpy installed.

@sizmailov
Copy link
Contributor Author

The problem is that generated signatures are not valid in sense of interpretation as stubs signatures. nan refers to a name which is not defined, so docgen/static analysis tools gets confused. The numpy.nan can be replaced with float('nan'), but I think it's a bit more readable.

The situation is worse when default value is not even valid python expression, e.g. x: Foo = <Foo instance 0xffff>. I think it needs generic solution so I close this PR. In shed of light #2244 closing I think you will argue against any PRs in this direction too.

Documentation matters therefore I think it's worth to produce toolable docstrings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants