Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I354 cpp docs #355

Merged
merged 23 commits into from
Nov 24, 2021
Merged

I354 cpp docs #355

merged 23 commits into from
Nov 24, 2021

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Nov 23, 2021

This PR starts the process of documenting the C++ interface, which was getting increasingly awkward to do from vignettes.

  • Basic doxygen file which outputs xml
  • Sphinx + breathe to assemble that into something reasonable
  • Splice that into the pkgdown docs

There's quite an exciting system for creating automatic examples, but I think it works pretty well. It's possible that either sphinx or doxygen supports this more natively but it's not obvious to me.

The main outstanding pain now is that it would be (much) nicer if we could sort out getting sphinx to inherit the pkgdown theme. That might not be that hard to do given they're both bootstrap based, but I'm not giving it a try yet.

Lots of points of improvements in the docs I expect, but I believe I've captured the key bits. We'll need to add another round here for the GPU interface probably, and I was thinking that the main dust objects would benefit from one too before starting on any python interface (#271). However, that can wait too.

Once this is up I will describe it in a blog post. Much of this approach came from this blog post.

Other things that snuck in while being documented:

  • Our DUST_VERSION_STRING was incorrectly not a string
  • The import_state method in prng was unnecessarily general and I've tidied that up

The nominally excessive size is due to the crazy default Doxyfile - I had a quick go at hacking it down but broke things on the first attempt. I'll probably have another go tomorrow but that should not hold anything else up Another attempt and we've got a sensible size Doxyfile now

Fixes #354

@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #355 (9fdf2a0) into master (b1bcb0b) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9fdf2a0 differs from pull request most recent head 4887096. Consider uploading reports for the commit 4887096 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master      #355   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         3508      3506    -2     
=========================================
- Hits          3508      3506    -2     
Impacted Files Coverage Δ
inst/include/dust/r/random.hpp 100.00% <ø> (ø)
inst/include/dust/random/binomial.hpp 100.00% <ø> (ø)
inst/include/dust/random/exponential.hpp 100.00% <ø> (ø)
inst/include/dust/random/generator.hpp 100.00% <ø> (ø)
inst/include/dust/random/multinomial.hpp 100.00% <ø> (ø)
inst/include/dust/random/normal.hpp 100.00% <ø> (ø)
inst/include/dust/random/poisson.hpp 100.00% <ø> (ø)
inst/include/dust/random/uniform.hpp 100.00% <ø> (ø)
inst/include/dust/random/prng.hpp 100.00% <100.00%> (ø)
inst/include/dust/random/xoshiro_state.hpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1bcb0b...4887096. Read the comment docs.

@richfitz richfitz requested a review from johnlees November 23, 2021 18:07
@richfitz richfitz marked this pull request as ready for review November 23, 2021 18:07
branches:
- master
- main
- i354-cpp-docs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly remove this before merging

@@ -35,6 +35,8 @@ And several on the random number generator, around which dust is built:
* `vignette("rng_algorithms")` on the details of algorithms used to sample from some distributions
* `vignette("rng_package")` on using the generators from other packages

The C++ API is documented in a separate [set of documentation](cpp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, this link going in one of the drop-down menus in the R docs would be very helpful for its discoverability I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's not going to be possible with this setup. As I was writing a blog post though I thought of a better way, and will implement that after. It will remove sphinx and breathe entirely from this system and create a set of additional markdown files that should easily be incorporated into the usual doc structure

developing.md Outdated
There are lots of places to consider putting documentation

* Following R package standards, all user facing functions must be documented and have examples, we use roxygen for this (generates files in `man/`). However, because most of the work in dust
* Vignettes form the backbone of the documentation. Mostof these are directly built as normal (in `vignettes/`) but a few are precomputed (see [`vignettes_src/`](vignette_src)) where they need special resources such as access to a GPU
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Vignettes form the backbone of the documentation. Mostof these are directly built as normal (in `vignettes/`) but a few are precomputed (see [`vignettes_src/`](vignette_src)) where they need special resources such as access to a GPU
* Vignettes form the backbone of the documentation. Mostof these are directly built as normal (in `vignettes/`) but a few are precomputed (see [`vignettes_src/`](vignette_src)) where they need special resources such as access to a GPU or using many cores.

developing.md Outdated

There are lots of places to consider putting documentation

* Following R package standards, all user facing functions must be documented and have examples, we use roxygen for this (generates files in `man/`). However, because most of the work in dust
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial sentence at the end here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea what that was going to say

inst/include/dust/random/exponential.hpp Outdated Show resolved Hide resolved
sphinx/random-r.rst Outdated Show resolved Hide resolved
sphinx/random-r.rst Outdated Show resolved Hide resolved
sphinx/random-r.rst Outdated Show resolved Hide resolved
.. doxygenenum:: dust::random::scrambler


Concrete types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.dcpu1.com/blog/different-types-of-concrete/?

Maybe something like 'Random number generator algorithms'

@@ -51,7 +51,7 @@ and then run it with

```r
pi_r(1e6)
#> [1] 3.139764
#> [1] 3.139504
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I enjoy that pi changes with every re-run, which confuses me when I first see it

@richfitz richfitz merged commit 2dfae09 into master Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document C++ interface properly
2 participants