Repository navigation
[pyos][docs] API Docs missing large amount of user-facing functions/methods/classes #410
Description
As-is, only the top-level classes are included in the API documentation, which seems to leave out most of the user-facing functions if i'm not mistaken?
Eg. I was trying to figure out how I would know about the various classes/functions used in the example notebooks, like i saw binary.truediv in the pagerank example, and there is nothing like that in the API docs.
It seems like the place these are documented are here (for operators): https://python-graphblas.readthedocs.io/en/stable/user_guide/operators.html but that list is hardcoded and so misses a decent amount of the available binary operators (table at bottom of issue).
Ideally these would all be listed in the API docs, which I think would require a refactor of the docs ( #409 ).
Since this package is a wrapper of a few different packages, it doesn't seem practical or useful to document every single one of these functions/classes, since presumably the wrapped libraries have their own documentation. It should be possible to list them and link to the original docs, however, and that seems like a pretty necessary thing to have in place to make this broadly usable by people who don't already know GraphBLAS from other contexts (I'm assuming these are all standard across GraphBLAS distributions?).
Aside from the wrapped functions/classes, there are a large number of functions/classes that lack a docstring (351 by an inspection that also includes trivially missing docstrings like module-level docstrings. You all will have a better sense of what counts as "user-facing" in this context, but presumably some subset of those are intended for use by the user.
From a developer/contributor POV, this additional documentation would be necessary for me to contribute to the package, as currently I am trying to make sense of the structure of the package, and some narrative docs in, for example, the positively massive core/operator.py file would go a long way in helping me understand that. Again, understanding this is a wrapper package which requires a lot of metaprogramming, so some documentation concerns don't apply since the functions/methods are generated dynamically, but as is I don't see a way for me to understand how that metaprogramming works. Perhaps that should be a separate issue though.
dir(graphblas.binary) |
docs |
|---|---|
| 'absfirst' | |
| 'abssecond' | |
| 'any' | |
| 'atan2' | 'atan2' |
| 'band' | 'band' |
| 'bclr' | |
| 'bget' | |
| 'binom' | |
| 'bor' | 'bor' |
| 'bset' | |
| 'bshift' | |
| 'bxnor' | 'bxnor' |
| 'bxor' | 'bxor' |
| 'cdiv' | |
| 'cmplx' | |
| 'copysign' | |
| 'eq' | 'eq' |
| 'first' | 'first' |
| 'firsti' | |
| 'firsti1' | |
| 'firstj' | |
| 'firstj1' | |
| 'floordiv' | |
| 'fmod' | 'fmod' |
| 'from_string' | |
| 'ge' | 'ge' |
| 'gt' | 'gt' |
| 'hypot' | |
| 'isclose' | |
| 'iseq' | |
| 'isge' | |
| 'isgt' | |
| 'isle' | |
| 'islt' | |
| 'isne' | |
| 'land' | 'land' |
| 'ldexp' | |
| 'le' | 'le' |
| 'lor' | 'lor' |
| 'lt' | 'lt' |
| 'lxnor' | 'lxnor' |
| 'lxor' | 'lxor' |
| 'max' | 'max' |
| 'min' | 'min' |
| 'minus' | 'minus' |
| 'ne' | 'ne' |
| 'numpy' | |
| 'pair' | 'pair' |
| 'plus' | 'plus' |
| 'pow' | 'pow' |
| 'rdiv' | |
| 'register_anonymous' | |
| 'register_new' | |
| 'remainder' | |
| 'rfloordiv' | |
| 'rminus' | |
| 'rpow' | |
| 'rtruediv' | |
| 'second' | 'second' |
| 'secondi' | |
| 'secondi1' | |
| 'secondj' | |
| 'secondj1' | |
| 'ss' | |
| 'times' | 'times' |
| 'truediv' | 'truediv' |
Related to: #409
Part of: pyOpenSci/software-submission#81
Activity
eriknw commentedon Mar 20, 2023
I agree it would be great to more thoroughly document the operators such as from
gb.binary. We can/should probably even group them as "builtin, part of the spec", "builtin, SuiteSparse:GraphBLAS-specific", and UDF. We can include a lot of relavent information in a table view for the different operators, and we can even indicate what the numpy-equivalent is when possible (since we have this info in e.g.gb.binary.numpynamespace). Whatever we do though would need to be autogenerated.A few of them, yes, but it's not quite as dire as it looks at a glance. I commented out the following ignore codes and ran
ruffviapre-commit run --all-files:python-graphblas/pyproject.toml
Lines 248 to 255 in c3baea8
This probably has similar results to your inspection. When looking through the results, I see very little that I regard as user-facing or particularly important. It also doesn't recognize that
@wrapdocdecorator adds a docstring. To be sure, though, there are a few user-facing methods and properties such as.new(...)that could use docstrings, and we can do better in general. If you see any specific methods, functions, properties, or error messages that should be added or improved during the course of your experiments, please comment here or raise new issues :)Heh, every time I've tried to split it up, I've failed. Here's a quick sketch of the important bits: we call e.g.
UnaryOp._initialize, which then does a regex onlib(the objects from cffi that wraps the C implementation) to find unary operators, then we add all the typed unary operators (asTypedBuiltinUnaryOp) toUnaryOp, which we save togb.unary. We do the same thing for binary operators, monoids, semirings, and indexunaryops.eriknw commentedon Mar 27, 2023
@sneakers-the-rat, please let us know if there is anything we can do (chat, meet, reply to issues, etc.) to help you understand our project and perform the review more effectively.
szhorvat commentedon Jun 25, 2023
Most of these can be looked up in the GraphBLAS spec/API, as Erik mentioned, but I'd also have liked to see them directly in the python-graphblas docs.
However, there are things which are specific to this package and are not in the docs, such as the
viznamespace.io.drawis documented, but it shows a warning saying that it's deprecated in favour ofviz.draw.viz.drawdoes not appear in the API docs or guide.sneakers-the-rat commentedon Jun 27, 2023
So I actually just tried to make this work for an hour or so using autodoc.
operator methods are all present in the
dir(operator)call, but also each of the operator modules overrides__dir__, so that might be something.I tried the following automodule commands:
And only the last one gave a sort of partial documentation here:
(zip of the generated html file, it's missing all the stylesheets and whatnot, but the content of the html is what's important)
operator_methods.html.zip
So it seems to get some of them, but some of the methods are also in the
_delayedmodule-level dict, and I ran out of time to chase down the other indirection methods.This is I think related to both
#433
and
#429
The thing that is making this difficult is that I'm still not sure if there is a single source of truth for all the available operator names. I understand that they are generated at import time, but it should be possible for me to, say, call
binary.list_operators()or something and get a definitive list of all possible operators, whether that be string representations, the actual objects, or both. As is, callingdir()or using tab completions is suboptimal sincedir()returns all the globals as well, and tab completion requires you to hunt around in dir.If there is such a thing, then documenting all available directives would just be a matter of making a new sphinx directive that calls
autoclassfor each of the operators. Or, if that's out of reach, simply pprinting the result of that function in the docs would do.It is a small thing relative to the scale of the whole package, but it actually is a pretty important thing from a user discovery POV, and I feel like it shouldn't be too hard to implement by the devs who understand the abstraction of how the operators are registered better than I do
eriknw commentedon Jul 11, 2023
Awesome, thanks for taking the time to explore this! This looks great (up until it stopped working) and is a good start.
I like the idea of having a definitive set of names of default operators.
__dir__gets us close, and the following should work for all the operator types:{x for x in dir(gb.unary) if not x.startswith('_')} - {'register_new', 'register_anonymous', 'ss', 'from_string', 'numpy'}I think there probably should be a global such as
DEFAULT_NAMESor whatever. (It's late, and I'm bad at coming up with names when it's late)The discussion from this issue probably has a few action items. We should probably enumerate them and/or make new issues as appropriate.
Add missing entries to the API reference (GH #410, #541)
Add missing entries to the API reference (GH #410, #541)
Docs: fix examples, fill API reference gaps, document gotchas (#602)