Expand and disambiguate the matrix/vector/scalar functional API#145
Closed
michelp wants to merge 5 commits intoGraphBLAS:mainfrom
Closed
Expand and disambiguate the matrix/vector/scalar functional API#145michelp wants to merge 5 commits intoGraphBLAS:mainfrom
michelp wants to merge 5 commits intoGraphBLAS:mainfrom
Conversation
Previously matrix.py, vector.py, and scalar.py only exposed `set_bool` and `bool` functions for accessing elements, even though SuiteSparse:GraphBLAS supports many more types. This fills out the functional API to cover every type the C library supports, following the existing `set_bool` / `bool` pattern exactly. Adds setter/getter pairs for: int8, int16, int32, int64, uint8, uint16, uint32, uint64, fp32, fp64, fc32, fc64. The complex types (fc32, fc64) are gated on `supports_complex()` so the functions are only defined on builds compiled with SuiteSparse complex support enabled (fc32/fc64 are GxB extensions, and the standard suitesparse.sh build disables them via GxB_NO_FC32 / GxB_NO_FC64). Each new function carries a doctest that round-trips a value through its setter and getter. Doctest counts after this change: matrix: 97 attempted, 0 failed vector: 90 attempted, 0 failed scalar: 81 attempted, 0 failed total: 268 attempted, 0 failed
The Dockerfile had bit-rotted: the python:3.10-slim-buster base image
no longer builds (Debian buster apt repos are gone), and the most recent
commit simultaneously dropped Python <=3.10 support in pyproject.toml
while leaving the Dockerfile pinned to Python 3.10. Several install
paths and cmake flags were also out of date for SuiteSparse:GraphBLAS
10.x.
Changes:
- Bump base image from python:3.10-slim-buster (EOL) to
python:3.12-slim-bookworm. Update hardcoded python3.10 site-packages
paths to python3.12 in three places.
- Update GraphBLAS header copy: 10.x installs the header at
/usr/include/suitesparse/GraphBLAS.h, not /usr/include/GraphBLAS.h,
and create_headers.py looks for it at
${sys.prefix}/include/suitesparse/GraphBLAS.h.
- Replace deprecated `python3 setup.py install` with
`pip install --no-build-isolation --no-deps .`. The pip-driven path
is required by modern setuptools, and pip is added to the build-time
install list along with pycparser/setuptools/wheel/setuptools-git-versioning
which the build now needs explicitly.
- Add cmake flags matching CI in suitesparse.sh:
-DCMAKE_BUILD_TYPE=Release, -DJITINIT=2, -DGRAPHBLAS_USE_JIT=OFF.
The JIT flags avoid segfaults in tests; without them GraphBLAS may
attempt JIT compilation at runtime.
- Replace stale -DGBCOMPACT cmake flag with the current name -DCOMPACT,
and give ARG COMPACT a default of 0 so the substitution is non-empty
when the build-arg is omitted.
- Make `git tag ${VERSION}` idempotent: wrap with `(... || true)` so
the build does not fail when ${VERSION} matches an existing tag in
the source tree (which is now the case for the current 10.3.1.0
release tag).
- Fix Docker COPY collapsing the libgraphblas symlink chain
(libgraphblas.so -> .so.10 -> .so.10.3.1) into three identical 67MB
regular files. Copy only the real versioned library
(libgraphblas.so.*.*.*) and recreate symlinks via `ln -sf` in a RUN
step. Apply the same fix to libgomp in the final stage. This removes
the `ldconfig: ... is not a symbolic link` warnings and saves ~130MB.
- Cleanup: remove unused `ARG SUITESPARSE` redeclaration in the psg
stage, fix the lowercase `as` -> `AS` for FromAsCasing warnings,
and switch `ENV PYTHONUNBUFFERED 1` to the modern `ENV key=value`
form.
- Drop the bizarre `WORKDIR /build/GraphBLAS/build` that was set
*before* the GraphBLAS git clone, producing the path
/build/GraphBLAS/build/GraphBLAS/build. The clone now happens
directly under /build.
Built and verified by running the doctests inside the produced image:
$ docker build --build-arg SUITESPARSE=v10.3.1 \\
--build-arg VERSION=10.3.1.0 \\
-t psg-test:latest .
$ docker run --rm psg-test:latest python3 -c "
import suitesparse_graphblas; suitesparse_graphblas.initialize()
from suitesparse_graphblas.tests.test_doctest import test_run_doctests
test_run_doctests()"
-> 268 doctests pass, supports_complex=True
The element getter functions in matrix.py, vector.py, and scalar.py were named after their GraphBLAS type suffix — `bool`, `int8`, `fp32`, `fc64`, etc. — which can be confused with concrete type objects (e.g. NumPy's `int8` dtype). Prefix every getter with `get_`, mirroring the existing `set_` prefix on the corresponding setters: `bool` -> `get_bool`, `int32` -> `get_int32`, `fc64` -> `get_fc64`, etc. The setters (`set_bool`, `set_int8`, ...) are NOT renamed. This is a pure rename, not an API redesign. Implementations and doctests are otherwise unchanged. Each setter's doctest already referenced its corresponding getter for round-trip verification, so both setter and getter docstrings were updated. Affected: - 13 getter `def` lines per file (one per element type) - 26 doctest references per file (one in each setter doctest, one in each getter doctest) - 3 files * (13 + 26) = 117 line replacements total Verified by rebuilding the Docker test image and rerunning the full doctest suite inside the container: matrix: 97 attempted, 0 failed vector: 90 attempted, 0 failed scalar: 81 attempted, 0 failed TOTAL: 268 attempted, 0 failed test_run_doctests: OK Both the direct `doctest.testmod` invocation and the official `tests/test_doctest.py::test_run_doctests` entry point pass, including the complex `get_fc32` / `get_fc64` doctests that depend on `supports_complex()`.
The metadata/utility helpers in matrix.py, vector.py, and scalar.py
were named after generic concepts (`new`, `free`, `type`, `format`,
`shape`, `size`, etc.). Several of those names shadow Python builtins
(`type`, `format`) or collide with common variables, making bare
imports awkward (`from suitesparse_graphblas.matrix import type` is
asking for trouble). Prefix every helper with its module name so each
function is unambiguous on import:
matrix.py (16 helpers):
free / new / type / nrows / ncols / nvals / shape / format /
set_format / sparsity_status / sparsity_control /
set_sparsity_control / hyper_switch / set_hyper_switch /
bitmap_switch / set_bitmap_switch
-> matrix_free, matrix_new, matrix_type, ...,
matrix_set_bitmap_switch
vector.py (5 helpers):
free / new / type / size / nvals
-> vector_free, vector_new, vector_type, vector_size, vector_nvals
scalar.py (3 helpers):
free / new / type
-> scalar_free, scalar_new, scalar_type
The element setters/getters (set_bool, get_bool, set_int8, get_int8,
..., set_fc64, get_fc64) are NOT renamed -- they already have a clear
set_*/get_* naming convention.
Internal cross-references updated:
- `def matrix_new(T, ..., *, free=matrix_free)` -- the default value of
the `free` keyword argument now references the renamed module-level
function. The kwarg parameter name itself stays as `free` so existing
callers passing `free=None` or `free=my_func` still work.
- `matrix_shape()` body now calls `matrix_nrows(A)` and `matrix_ncols(A)`.
- All in-module doctests updated: `>>> A = new(...)` -> `>>> A = matrix_new(...)`,
`>>> nrows(A)` -> `>>> matrix_nrows(A)`, etc. Element setter/getter
doctests that call `new(...)` to construct test instances are also
updated to use the prefixed name.
- Local variable shadows (e.g. `format = ffi.new(...)` inside
`matrix_format()`) and kwarg parameter names (e.g. `format` in
`matrix_set_format(A, format)`) are intentionally left alone -- they
are local and don't reference the renamed functions.
- `ffi.new(...)` (cffi calls) is preserved -- distinguished from the
renamed `new(...)` via a negative lookbehind in the rename pattern.
External call sites updated:
- `suitesparse_graphblas/__init__.py`: `burble` class doctest
references `matrix.matrix_new(...)` and `matrix.matrix_nvals(...)`.
- `suitesparse_graphblas/io/binary.py`: 13 `matrix.X(...)` call sites
in the binread/binwrite serialization paths now call
`matrix.matrix_X(...)`.
- `suitesparse_graphblas/io/serialize.py`: 2 docstring prose mentions
plus 2 attribute references (`matrix.free` -> `matrix.matrix_free`,
`vector.free` -> `vector.vector_free`) used as the default GC
callback in `deserialize_matrix` / `deserialize_vector`.
- `suitesparse_graphblas/tests/test_io.py`: 17 external call sites
updated to the prefixed names.
Verified by rebuilding the Docker test image and running both the full
doctest suite and the pytest suite inside the container:
matrix: 97 attempted, 0 failed
vector: 90 attempted, 0 failed
scalar: 81 attempted, 0 failed
TOTAL doctest: 268 attempted, 0 failed
pytest: 9 passed
test_doctest, test_exceptions, test_io (matrix and vector
serialization round-trip), test_jit, test_package, test_scalar
The `matrix.nvals` -> `matrix.matrix_nvals` rename pushed the triple equality in `test_matrix_binfile_read_write` to 101 characters, exceeding the project's 100-char line length. Apply Black's suggested parenthesized wrap to bring it back under the limit.
Member
Author
|
I'm going to rescind this pr for now for some more upstream changes after discussion. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Expands the
matrix.py/vector.py/scalar.pyfunctional API to cover every GraphBLAS element type, gives every function an unambiguous name, and brings theDockerfileback to a working state on current Python and GraphBLAS.Commits
61813caAddset_*/get_*element accessors for every GraphBLAS type (int8…fc64). Complex types are gated onsupports_complex(). Each function ships with a round-trip doctest.a85dc40Modernize theDockerfile: bump base topython:3.12-slim-bookworm, fix GraphBLAS 10.x install paths, add JIT cmake flags matchingsuitesparse.sh, recreatelibgraphblas/libgompsymlinks (DockerCOPYwas collapsing the chain into duplicate 67MB files), makegit tagidempotent, fixas/ASand legacy-ENVwarnings.a062434Prefix element getters withget_(bool→get_bool,int8→get_int8, …,fc64→get_fc64) so they don't shadow type identifiers like NumPy'sint8.927e430Prefix the metadata helpers in each module with the module name (new→matrix_new,free→matrix_free,type→matrix_type, …) so they don't shadow Python builtins. Element setters/getters keep theirset_*/get_*convention. External callers inio/binary.py,io/serialize.py,__init__.py, andtests/test_io.pyare updated accordingly.Test plan
Verified inside a freshly-rebuilt Docker test image against
SUITESPARSE=v10.3.1:test_io.pymatrix and vector serialization round-trip)