llnl / llnl/pylibROM

Required fixes

Open
#1 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
63
Forks
3
PR merge metrics
No merged PRs in 30d

Description

### ~~Default values not properly declared~~
If the function takes a default value in its arguments, it needs to be specified with `py::arg`. For example of `Vector` constructor,
```
// Constructor
.def(py::init([](py::array_t vec, bool distributed, bool copy_data = true) {
py::buffer_info buf_info = vec.request();
int dim = buf_info.shape[0];
double* data = static_cast(buf_info.ptr);
return new Vector(data, dim, distributed, copy_data);
}), py::arg("vec"), py::arg("distributed"), py::arg("copy_data") = true) // default value needs to be defined here.
```
Note the last line.

### ~~`Vector::getData` not returning an array object~~
In c++, `getData` function returns the memory address of the data array. In python, this should return the reference of the data array. Currently, `Vector::getData` does not do any special care, returning a `float` value as a result.
```
.def("getData", &Vector::getData)
```

### `Vector::get_data` naming
Compared to the function above, this function can cause a confusion. Based on its action, it needs to be renamed as `copy_data` or something.

### ~~(Enhancement) `Vector` and `Matrix` as a buffer protocol~~
`Vector` and `Matrix` should support buffer view, to handler large-size arrays without copying them in memory. Useful references are:
- [python buffer protocol](https://docs.python.org/3/c-api/buffer.html)
- [pybind11 numpy instruction](https://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html)

Based on [pybind11 numpy instruction](https://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html), an example of `Matrix` definition should be:
```
py::class_(m, "Matrix", py::buffer_protocol())
.def_buffer([](Matrix &self) -> py::buffer_info {
return py::buffer_info(
self.getData(), /* Pointer to buffer */
sizeof(float), /* Size of one scalar */
py::format_descriptor::format(), /* Python struct-style format descriptor */
2, /* Number of dimensions */
{ self.numRows(), self.numColumns() }, /* Buffer dimensions */
{ sizeof(float) * self.numColumns(),
sizeof(float) } /* Strides (in bytes) for each index */
);
})
```

Similar definition is possible for `Vector` as well.

### (Enhancement) Supporting slice view of `Vector` and `Matrix`
`Vector` and `Matrix` must support slice view of its data.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating the Vector and Matrix pybind11 bindings and compare their current APIs with the issue's naming and slice-view requirements. Use the linked Python buffer protocol and pybind11 NumPy references as the design starting point. Done means the remaining naming concern and slice-view support are resolved for both types and verified by relevant tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, python
Domain
api, backend
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.