BallisticLA / RandBLAS

A header-only C++ library for sketching in randomized linear algebra
https://randblas.readthedocs.io/en/latest/
Other
66 stars 4 forks source link

Revamp web documentation. Also, simplify constructor APIs for DenseSkOp, SparseSkOp, RNGState objects. #37

Closed rileyjmurray closed 1 year ago

rileyjmurray commented 1 year ago

Changes to docs

This PR includes source documentation for basic dense and sparse sketching functionality. It also overhauls our sphinx web documentation.

The source docs are written with consideration to our use of sphinx-breathe. The sphinx-breathe functionality has been extended with a new reStructuredText directive, mathmacro, that lets you declare LaTeX macros anywhere in source docs or web docs. There is an important side-effect of using sphinx-breathe in this way: it makes the plain Doxygen website borderline unreadable. Unfortunately, I think we might have to accept this as inevitable (i.e., we can't hope for both a clean reStructuredText based website and a clean Doxygen site). I'm open to exploring alternative solutions so that we might have a proper Doxygen site as well, but for now, I want to set that issue aside.

Changes to code

This PR also removes a bunch of overloaded constructors for SparseSkOp and DenseSkOp objects. Most of the constructors just let you unpack a structure argument. That is, they let you call f(a, b, c) instead of f(s), where s is a struct with fields a, b, c. The better solution here is just to tell users to use a struct literal: f({a, b, c}).

The build is failing due to known numerical issues in macOS tests.

TODOs

@burlen please review this PR when you get a chance. Also, can you remind me if you've done any work on flattening the RandBLAS namespaces? This PR doesn't include any such work. So if you have some staged somewhere, then I want to be cognizant of merge conflicts.

I haven't written the docstrings for RandBLAS::dense::fill_buff or RandBLAS::dense::fill_skop_buff.

Something to consider for the future

The documentation for LSKGE3 is almost identical to LSKGES. This is intentional but perhaps not ideal. It suggests that we might define a new function, LSKGEX, that fully templates the sketching operator S so as to allow either SparseSkOp or DenseSkOp objects. The implementation could then dispatch LSKGE3 or LSKGES, depending on the type of S. In future versions of RandBLAS that support SRFT sketching operators, we could also have LSKGEX dispatch the corresponding implementation for SRFT sketching.

burlen commented 1 year ago

@rileyjmurray Exciting! I did not flatten RandBLAS namespaces yet, so this is good timing as far as I'm concerned. I'll take a closer look at this first thing in the morning.

burlen commented 1 year ago

@rileyjmurray there seems to be some issues with missing files in rtd.org. https://readthedocs.org/projects/randblas/builds/19631164/

What is your RTD user name? If you don't have one please make one and send to me so I can add you as a maintainer. Then you can look at the error messages in the above link.

burlen commented 1 year ago

https://github.com/BallisticLA/RandBLAS/pull/37/commits/c29c0bd1cbf029af9c0df7190e87a7370dda91c2 fixed the issue. see https://randblas.readthedocs.io/en/docs-for-lskges-lskge3/index.html