[WIP] Fix doxygen so it compiles with 0 errors and update docs #2745

SteveBronder · 2022-05-24T22:13:40Z

Summary

This cleans up a lot of the doxygen to remove a lot of the warnings we previously had. The biggest warning was about parameters being defined multiple times for different overloads. This was caused by doxygen parsing functions while ignoring non-type template parameters which we use for writing our overloads. For example, doxygen would look at both of these functions signatures as if they were the same

template <typename T, require_arithmetic_t<T>* = nullptr>
auto a_fun(const T& x);

template <typename T, require_var_t<T>* = nullptr>
auto a_fun(const T& x);

To fix this I just changed the names of the template parameters. When possible I would change the name to reflect, conceptually, what types would go into the overload. But in some cases there are several generic overloads and so for those I just added arbitrary named templates like T1, T2, T3, etc.

Tests

This should only be a cosmetic change so no new tests should be necessary.

The docs should be looked over from a local build of the website using make doxygen

Side Effects

Yes, doxygen now requires at least version 1.8.13. This is the default for Ubuntu in 20.04, but I'm not sure what version homebrew has. I also got this to make the docs with doxygen 1.9.1 but there could be errors I'm missing with newer versions like 1.9.4. Bullseye Debian only has version 1.8.3 so our docs will not compile on Bullseye debian systems with the basic install unless they install doxygen from backports.

Release notes

Update the documentation

Checklist

Math issue #(issue number)
Copyright holder: Steve Bronder

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- dependencies checks pass, (make test-math-dependencies)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

SteveBronder · 2022-05-24T22:15:39Z

@bob-carpenter I still need to finish some of the edits to the adding new functions documentation but this should be ready to read over by tmrw or so

SteveBronder · 2022-06-01T19:05:01Z

Alrighty @bob-carpenter I think I got everything sorted out. Can you try checking out this branch and running make doxygen? If you have time to look over the section on adding new functions I think I got most of the pieces you were had Qs about

bob-carpenter · 2022-06-01T21:12:06Z

The branch doesn't build for me in doxygen. Could I have the wrong version of doxygen installed?

~/github/stan-dev/cmdstan/stan/lib/stan_math (fix/docs1)$ doxygen --version
1.9.3

Here's the error dump from doxygen:

~/github/stan-dev/cmdstan/stan/lib/stan_math (fix/docs1)$ make -j8 doxygen
mkdir -p doc/api
doxygen -v
1.9.3
doxygen doxygen/doxygen.cfg
warning: Tag 'OUTPUT_TEXT_DIRECTION' at line 102 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'CLANG_ASSISTED_PARSING' at line 1114 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_ADD_INC_PATHS' at line 1120 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_OPTIONS' at line 1128 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_DATABASE_PATH' at line 1174 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'LATEX_SOURCE_CODE' at line 1941 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'RTF_SOURCE_CODE' at line 2031 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'DOCBOOK_PROGRAMLISTING' at line 2136 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'CLASS_DIAGRAMS' at line 2340 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
/Users/bcarpenter/github/stan-dev/cmdstan/stan/lib/stan_math/lib/opencl_3.0.0/CL/opencl.hpp:2061: error: the \endcond does not have a corresponding \cond in this file (warning treated as error, aborting now)
make: *** [doxygen] Error 1

SteveBronder · 2022-06-01T23:51:50Z

No this is an error when compiling with 1.9.1+, it is trying to document the OpenCL library we use as well which is odd. Let me see if I can tell it to not try to do that. Oddly this doesn't show up for <= 1.9.1

SteveBronder · 2022-06-02T15:58:46Z

Alrighty @bob-carpenter can you give it another shot?

bob-carpenter · 2022-06-02T16:59:55Z

Awesome. It works now with only some config warnings. Should I go ahead and review and merge?

~/github/stan-dev/cmdstan/stan/lib/stan_math (fix/docs1)$ make -j8 doxygen
mkdir -p doc/api
doxygen -v
1.9.3
doxygen doxygen/doxygen.cfg
warning: Tag 'OUTPUT_TEXT_DIRECTION' at line 102 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'CLANG_ASSISTED_PARSING' at line 1114 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_ADD_INC_PATHS' at line 1120 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_OPTIONS' at line 1128 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_DATABASE_PATH' at line 1174 of file 'doxygen/doxygen.cfg' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'LATEX_SOURCE_CODE' at line 1941 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'RTF_SOURCE_CODE' at line 2031 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'DOCBOOK_PROGRAMLISTING' at line 2136 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'CLASS_DIAGRAMS' at line 2340 of file 'doxygen/doxygen.cfg' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
cp ./doxygen/pretty_stuff/eigen_navtree_hacks.js ./doc/api/html
~/github/stan-dev/cmdstan/stan/lib/stan_math (fix/docs1)$

SteveBronder · 2022-06-02T19:50:08Z

If you can start reviewing the doxygen docs I wrote that would be great, but I need to check out what the error is coming from on Jenkins right now in prim

bob-carpenter · 2022-06-02T20:52:13Z

It looks like the error may be on Jenkins config.

I don't know how to mechanically do the review here. The 400+ files just chokes my web browser and the web page takes forever to scroll (30+ seconds) or to click (60+ seconds). Suggestions?

SteveBronder · 2022-06-03T03:10:39Z

Oh wow, so on my desktop its fine but on my laptop it totally crashes. Can you try the link below while also setting the diff to unified view?

https://github.com/stan-dev/math/pull/2745/files?file-filters%5B%5D=.cfg&file-filters%5B%5D=.css&file-filters%5B%5D=.h&file-filters%5B%5D=.html&file-filters%5B%5D=.js&file-filters%5B%5D=.md&file-filters%5B%5D=.xml&show-viewed-files=true&w=1

Both of those things managed to get the docs up and reviewable. I think unified view also works for reviewing the code

bob-carpenter · 2022-06-03T16:44:43Z

Thanks, @SteveBronder---that works. Now onto the actual reviewing.

bob-carpenter

Well that took several hours. I can't imagine how long this took to write. Thanks, it all looks amazing. Almost all of my comments are minor grammatical and formatting things.

But what I don't see is the changed C++ code. Is that coming in a different PR?

bob-carpenter · 2022-06-03T18:43:53Z

doxygen/README.md

+
+int main() {
+  std::cout << "log normal(1 | 2, 3)="
+            << stan::math::normal_log(1, 2, 3)


This should be _lpdf---the _log suffixes are deprecated (or should be, if they're not).

bob-carpenter · 2022-06-03T18:45:55Z

doxygen/README.md

+```
+
+If this is in the file `/path/to/foo/foo.cpp`, then you can compile
+and run this with something like this, with the `/path/to` business


I'd just say <path>/foo.cpp and mention that it's a path.

I'd also prefer to stay away from vague statements like "something like this"---if it's not exactly this, then just say how it differs (e.g., replace <path> with an actual path).

bob-carpenter · 2022-06-03T18:48:27Z

doxygen/contributor_help_pages/common_pitfalls.md

-it new meta-programs should be added.
-
-
+All of the SFINAE template meta-programs in Math are special to Stan or Math. Documentation can be found in the @ref require_meta docs.


no hyphen in metaprogram. don't listen to your spellchecker if it tries to tell you otherwise.

Ty! I'll look at other places this is done as well

bob-carpenter · 2022-06-03T18:50:38Z

doxygen/contributor_help_pages/common_pitfalls.md

-the expression it is used in.
+[godbolt example](https://godbolt.org/z/6Te856Mv7)
+
+When we return back an **eigen expression** containing objects created in the local scope we need a way to store those locally created objects till the expression is evaluated.


capitalize Eigen; return back -> return

bob-carpenter · 2022-06-03T18:52:14Z

doxygen/contributor_help_pages/common_pitfalls.md

    other.m_data = nullptr;
  }
 };
 ```

 We can see in the above that the standard style of a move (the constructor taking an rvalue reference) is to copy the pointer and then null out the original pointer. But in Stan, particularly for reverse mode, we need to keep memory around even if  it's only a temporary for when we call the gradient calculations in the reverse pass. And since memory for reverse mode is stored in our arena allocator no copying happens in the first place.

-When working with arithmetic types, keep in mind that moving Scalars is often less optimal than simply taking their copy. For instance, Stan's `var` type is a PIMPL implementation, so it simply holds a pointer of size 8 bytes. A `double` is also 8 bytes which just so happens to fit exactly in a [word](https://en.wikipedia.org/wiki/Word_(computer_architecture)) of most modern CPUs. While a reference to a double is also 8 bytes, unless the function is inlined by the compiler, the computer will have to place the reference into cache, then go fetch the value that is being referenced which now takes up two words instead of one!
+When working with arithmetic types, keep in mind that moving Scalars is often less optimal than simply taking their copy. For instance, Stan's `var` type is a PIMPL implementation, so it simply holds a pointer of size 8 bytes. A `double` is also 8 bytes which just so happens to fit exactly in a [word](https://en.wikipedia.org/wiki/Word_(computer_architecture)) of most modern CPUs with at least 64 byte cache lines. While a reference to a double is also 8 bytes, unless the function is inlined by the compiler, the computer will have to place the reference into cache, then go fetch the value that is being referenced which now takes up two words instead of one!


I think we should avoid these super long lines.

But the reason I opened this box was to say that "PIMPL impementation" is redundant because the IMPL stands for "implementation. So I'd say "uses the pointer to implementation (PIMPL) pattern" to avoid redundancy.

Plus, thanks for explaining the "why" here---I hadn't realized references would tickle two words instead of one. I would've expected that with a pointer, but I would've thought references would be smarter given that they're immutable.

I think we should avoid these super long lines.

Yeah I'll add some returns here to make the markdown easier to read.

But the reason I opened this box was to say that "PIMPL impementation" is redundant because the IMPL stands for "implementation. So I'd say "uses the pointer to implementation (PIMPL) pattern" to avoid redundancy.

Yes thats a much better way to word it, fixed!

Plus, thanks for explaining the "why" here---I hadn't realized references would tickle two words instead of one. I would've expected that with a pointer, but I would've thought references would be smarter given that they're immutable.

Yes, though note that this is one of those weird internal to compiler things that tbh will rarely come up, but with large programs you can have instances where this sort of thing happens.

bob-carpenter · 2022-06-03T21:25:01Z

doxygen/contributor_help_pages/require_meta.md

+  @ref stan::is_var.
+
+ The `requires` type traits allow Stan to have more generic types so that the
+ library can forward along Eigen expression and have better move semantics.


forward along -> forward

bob-carpenter · 2022-06-03T21:25:22Z

doxygen/contributor_help_pages/require_meta.md

+ template <typename Mat1, typename Mat2,
+  require_all_eigen_vt<is_arithmetic, Mat1, Mat2>* = nullptr>
+ inline auto a_func(Mat1&& m1, Mat2&& m2) {
+   check_nan(m1);


I think it's check_not_nan.

bob-carpenter · 2022-06-03T21:26:16Z

doxygen/contributor_help_pages/require_meta.md

+
+For a full list of predefined `requires` please see @ref require_meta.
+
+If you are adding a new type trait that returns has a `bool value` member you can add it to the set of known requires by using the macros @ref stan/math/prim/meta/require_helpers.hpp .


returns has -> returns

bool value member -> bool value (members don't get returned by functions)

macros -> macros defined in

Fixed. This should not have been about returns but instead about definint type traits that have a bool member named value.

bob-carpenter · 2022-06-03T21:27:39Z

doxygen/contributor_help_pages/require_meta.md

+```
+
+We can add the associated `requires` for the standard requires of `require_double_only_t` using @ref STAN_ADD_REQUIRE_UNARY macro.
+We supply the name to use in the require (`double_only`), The type trait name (`is_double_only`) and which group this type trait is in `require_stan_scalar_real`.


using -> using the

bob-carpenter · 2022-06-03T21:29:06Z

doxygen/pretty_stuff/eigen_navtree_hacks.js

+    } catch (err) {
+      setTimeout(arguments.callee, 10);
+    }
+  })();


I don't get what anything in this file is doing or the previous one, but I assume they just come in as boilerplate from elsewhere and don't need thorough review.

Yes this is a hack of the Eigen hack so that the table of contents in the bottom left part of the docs page is drawn nicely.

SteveBronder · 2022-06-03T21:50:32Z

Thank you for the thorough review! Yes it took quite a while to write but I think it should help folks in the future. I'll give this a look over the weekend

bob-carpenter · 2022-06-09T17:54:20Z

Seriously, you can take as much of that feedback as you want. It's great as it is, and I wouldn't want to block it on grammar, formatting, etc.

SteveBronder · 2022-06-24T19:34:56Z

Alright I think I got everything! At this point just need to do a find replace to add newlines after each sentence and remove some of the I / we usage. @bob-carpenter I'll ping you when I'm finished with those

syclik · 2023-01-03T21:04:45Z

@SteveBronder, I'm trying to resurrect this PR. I merged develop into it to the best of my ability.

SteveBronder and others added 13 commits May 12, 2022 15:34

fix docs

2cd7fd0

cleanup docs

dcf43ee

more docs

bfe4bf2

comments about writing new function

2b44347

Merge remote-tracking branch 'origin/develop' into fix/docs1

44b71bd

fixes for compiling docs

e9248de

update

72f8842

get ref links working

bdbcdf2

fix links

0a73498

fixup docs

3bd9ab0

update docs for requires and references in the markdown docs

81f9e75

Merge commit '81c983fc29510aeeec1c1a50c6a17977c14095f7' into HEAD

18ae071

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

9da5982

SteveBronder added 2 commits May 24, 2022 21:20

fix template param names

b2c4d3c

Merge branch 'fix/docs1' of github.com:stan-dev/math into fix/docs1

080ecad

This was linked to issues May 25, 2022

clean up Doxygen #2687

Open

clean up doxygen errors introduced by new metaprograms #1793

Open

SteveBronder and others added 8 commits May 25, 2022 15:46

update nolint for require macros

8bfd344

Merge remote-tracking branch 'origin/develop' into fix/docs1

64c0c2e

add nolint to macro lines

a739b2a

update require docs

f277e86

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

d8a5191

cleanups to work on doxygen 1.9.4

908f906

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

34c581b

remove old doxygen.cfg and fix copy opencl compiler error

9e37f9e

Fix bug in opencl docs

5758d2b

Merge remote-tracking branch 'origin/develop' into fix/docs1

d99d47c

bob-carpenter requested changes Jun 3, 2022

View reviewed changes

Merge remote-tracking branch 'origin/develop' into fix/docs1

3a53f44

SteveBronder and others added 8 commits June 24, 2022 16:59

update docs for comments

8528fd0

fix wording

55bce7d

update docs

66c5900

fix some pieces of docs

071b7ab

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

562d277

update to develop

2067d29

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

26168cf

Merge branch 'develop' into fix/docs1

e251f5d

SteveBronder closed this Oct 26, 2023

		it new meta-programs should be added.


		All of the SFINAE template meta-programs in Math are special to Stan or Math. Documentation can be found in the @ref require_meta docs.


		For a full list of predefined `requires` please see @ref require_meta.

		If you are adding a new type trait that returns has a `bool value` member you can add it to the set of known requires by using the macros @ref stan/math/prim/meta/require_helpers.hpp .

[WIP] Fix doxygen so it compiles with 0 errors and update docs #2745

[WIP] Fix doxygen so it compiles with 0 errors and update docs #2745

Conversation

SteveBronder commented May 24, 2022 • edited

Summary

Tests

Side Effects

Release notes

Checklist

SteveBronder commented May 24, 2022

SteveBronder commented Jun 1, 2022

bob-carpenter commented Jun 1, 2022

SteveBronder commented Jun 1, 2022

SteveBronder commented Jun 2, 2022

bob-carpenter commented Jun 2, 2022

SteveBronder commented Jun 2, 2022

bob-carpenter commented Jun 2, 2022

SteveBronder commented Jun 3, 2022

bob-carpenter commented Jun 3, 2022

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveBronder commented Jun 3, 2022

bob-carpenter commented Jun 9, 2022

SteveBronder commented Jun 24, 2022

syclik commented Jan 3, 2023

SteveBronder commented May 24, 2022 •

edited