DM-43906: Make gauss2d_fit more dev guide compliant #13
Conversation
python/lsst/gauss2d/fit/data.cc
Outdated
| .def("__getitem__", &Data::at) | ||
| .def("__len__", &Data::size) | ||
| // .def("__repr__", [](const Data &self) { return self.repr(true); | ||
| // }) |
There was a problem hiding this comment.
Did you intend to delete or uncomment this?
| @@ -0,0 +1,58 @@ | |||
| .. py:currentmodule:: lsst.gauss2d.fit | |||
There was a problem hiding this comment.
I hadn't realized the Python package was going to be lsst.gauss2d.fit rather than lsst.gauss2dfit. Does this actually work with lsst.gauss2d being provided by another git/eups package? In other cases where we have a Python package split across multiple git/eups packages, we have to put them at the same level, since there can only be one (e.g.) lsst/gauss2d/__init__.py (either the pkgutils one that splits sibling packages, or a regular one).
There's some chance that this could work if PYTHONPATH has the two package directories in one order, and not if they are in the other order.
|
|
||
| ``lsst.gauss2d.fit`` has Python bindings for classes using numpy-based single | ||
| and double precision arrays. Support for GSL arrays is forthcoming with | ||
| `DM-38617 <https://jira.lsstcorp.org/browse/DM-38617>`_. |
There was a problem hiding this comment.
I'd leave TODO entries like this out of the docs. It's too easy for them to become stale.
| @@ -36,7 +36,8 @@ | |||
|
|
|||
| PYBIND11_MODULE(_gauss2d_fit, m) { | |||
| m.doc() = "Gauss2DFit Python bindings"; | |||
| py::module::import("gauss2d"); | |||
| // Is this necessary? Apparently not | |||
| // py::module::import("lsst.gauss2d"); | |||
There was a problem hiding this comment.
The impact of having this can be subtle: if you have any signatures in this package that reference types defined in lsst.gauss2d, you'll get better docstrings and conversion behavior will be more likely to be correct if you have this import in place.
| @@ -0,0 +1,10 @@ | |||
| # Change Log | |||
|
|
|||
| All notable changes to this project will be documented in this file. | |||
There was a problem hiding this comment.
If you are the only developer this approach is probably fine but if there are going to be other people working on it consider using towncrier as is done in middleware.
There was a problem hiding this comment.
I thought about it but is it not mainly Python-oriented?
There was a problem hiding this comment.
It's nothing to do with python other than you can configure it in a pyproject.toml file. You run the towncrier command to combine the fragments into a single file.
There was a problem hiding this comment.
Hmm, ok. I'll consider it for a future ticket. Are there instructions/best practices for the middleware approach beyond the notes on DM-30291 and the doc/changes dir? I think I've only gone through the process of add a news item once.
This restructuring makes it easier to build and install standalone with pip, as hacky as it is.
Like for gauss2d, this is in prep for adding it to lsst_distrib. Although it will be added as a third party package to start, the dev guidelines are worth following anyway.