Skip to content
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

pydrake autodiff: Use user-defined dtype #8797

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented May 11, 2018

Checkbox in #8116

Requires:


This change is Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

+@jwnimmer-tri for feature review, please.


Review status: 0 of 29 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented May 11, 2018

Per f2f, I will consider pulling out the user-defined NumPy dtype out of our fork of pybind11, and put it into drake, perhaps under bindings/pybind_extensions, given that (a) it's C++14+, (b) there doesn't seem to be any upstream need for it atm (so not much reason to make it work with C++11), and (c) it's coupled to this development.

@jwnimmer-tri
Copy link
Collaborator

Reviewed 19 of 29 files at r1.
Review status: 19 of 29 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 2 of 29 files at r1.
Review status: 21 of 29 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 3 of 29 files at r1.
Review status: 24 of 29 files reviewed at latest revision, all discussions resolved, some commit checks failed.


a discussion (no related file):
I think I will pause my review of this PR, until the pybind11 changes get code reviewed. I'd rather rely on that code being well-documented and tested before I review the balance of this PR that builds on those foundations.


bindings/pydrake/util/cpp_param_pybind.cc, line 82 at r1 (raw file):

      // aliasing / registering custom dtypes.
      std::type_index id(tinfo);
      auto* dtype_info = py::detail::dtype_info::maybe_get_entry(id);

TBD Reminder to myself to return here after the pybind11 PR is reworked to appear as part of //bindings instead.


bindings/pydrake/util/cpp_param_pybind.cc, line 93 at r1 (raw file):

      const std::string name = tinfo.name();
      throw std::runtime_error(
          "C++ type is not registered in pybind: " + name);

The unit test that covers this code does not check for any details about the exception. Given the amount of code in scope, it seems worth it to enhance that test to look at the exception messsage (so we know this is what threw), as opposed to some of the code we're calling throwing instead.


bindings/pydrake/util/wrap_pybind.h, line 84 at r1 (raw file):

}

/// Mirror ufunc loop definitions from NumPy to `math`

This comment is impenetrable and thus does not adequately explain what is going on here.

(1) Missing citation or explanation for what a ufunc is. (I assume its a numpy ufunc?)
(2) Unclear what "mirror"ing is, and why would a caller want to do it. (I assume this is sugar to help callers add functions in more than one place at once?)
(3) Unclear how and where "NumPy" comes into play. (I assume cls is supposed to be something numpy-ish?)


bindings/pydrake/util/wrap_pybind.h, line 84 at r1 (raw file):

}

/// Mirror ufunc loop definitions from NumPy to `math`

Why is the module named math here? It could be any module, right? (Also at the call site, it's not math but rather pydrake.math, so the name here seems misleading.)


bindings/pydrake/util/wrap_pybind.h, line 97 at r1 (raw file):

    math_.def(math_name, func);
    return *this;
  }

Missing docs for what the names are doing in the two different overloads.


bindings/pydrake/util/wrap_pybind.h, line 108 at r1 (raw file):

  PyClass* const cls_{};
  py::module math_;
};

FYI Consider placing this next to MirrorDef within this header -- they seem somewhat related, unlike WrapCallbacks.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Kicking back to @EricCousineau-TRI until its ready for review.

@EricCousineau-TRI
Copy link
Contributor Author

@jwnimmer-tri I've scoped the pybind11_ext (or whatever it'll be named) bits in #9997, which doesn't touch Drake stuff, it just shows what this stuff does itself.

I'd like to do the review there (at low priority, of course), and then dust this off whenever it lands. Would that work for you?
(I can still apply your suggestions in that code.)

@jwnimmer-tri
Copy link
Collaborator

I agree that #9997 review should happen prior to this PR. This PR is totally off my radar, so I'm still happy to keep ignoring it. If it turns into a useful supplement once other things land, I'm okay to review it at that point (just re-assign me).

@EricCousineau-TRI
Copy link
Contributor Author

Closing for now. Can re-open later.

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

Successfully merging this pull request may close these issues.

2 participants