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

Python API for place_pwlin. #1250

Merged
merged 5 commits into from
Dec 15, 2020

Conversation

halfflat
Copy link
Contributor

  • Add Python bindings for the place_pwlin and isometry classes.
  • Add equality test for Python mpoint.
  • Add unit tests for new Python interfaces.
  • Split C++ API morphology documentation into its own file.
  • Add C++ API and Python API documentation for place_pwlin and isometry.

Copy link
Contributor

@noraabiakar noraabiakar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! just one minor comment in the docs.

doc/cpp/morphology.rst Show resolved Hide resolved
doc/cpp/morphology.rst Outdated Show resolved Hide resolved
Copy link
Member

@bcumming bcumming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
The Python wrappers are missing docstrings.

Apply the isometry to the first three components of the given tuple, interpreted as a point.

.. py:function:: __mul__(a: isometry, b: isometry) -> isometry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is -> how one specifies the return type?
I have been using :rtype, e.g.:
https://github.com/arbor-sim/arbor/blame/master/doc/python/morphology.rst#L368

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just got that syntax from the Python type annotation syntax. I don't know if it's the right or even better way to do it though.

}),
"x"_a, "y"_a, "z"_a, "radius"_a, "All values in μm.")
.def(py::init<double, double, double, double>(),
"x"_a, "y"_a, "z"_a, "radius"_a)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring has been removed (which stated that all inputs were in um)

@@ -112,6 +116,68 @@ void register_morphology(py::module& m) {
.def("__str__", [](const arb::mcable& c) { return util::pprintf("{}", c); })
.def("__repr__", [](const arb::mcable& c) { return util::pprintf("{}", c); });

// arb::isometry
py::class_<arb::isometry> isometry(m, "isometry");
isometry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstrings are missing for the calls and methods in the isometry wrapper.

"theta"_a, "axis"_a);

// arb::place_pwlin
py::class_<arb::place_pwlin> place(m, "place_pwlin");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docstrings also for methods of place_pwlin.

* Add unit tests for isometry and place_pwlin Python bindings.
* Add equality test for mpoint.
* Add return types in cpp docs for isometry functions.
* Add missing docstrings in python interface for place_pwlin and isometry.
@halfflat halfflat force-pushed the feature/python-place-pwlin branch from 1c336cf to ff51a91 Compare December 14, 2020 16:36
@bcumming bcumming merged commit ff06a4d into arbor-sim:master Dec 15, 2020
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.

3 participants