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

Fix Doxygen warnings #2400

Merged
merged 40 commits into from May 16, 2020
Merged

Fix Doxygen warnings #2400

merged 40 commits into from May 16, 2020

Conversation

birm
Copy link
Member

@birm birm commented May 10, 2020

Resolution of Doxygen Warnings, Mostly wrong/missing/extra params, section renaming and latex/doxygen commands. Things that may require additional attention are flagged with comments.

@@ -344,9 +344,9 @@ For further documentation on the DTree class, consult the
The usual regularized error \f$R_\alpha(t)\f$ of a node \f$t\f$ is given by:
\f$R_\alpha(t) = R(t) + \alpha |\tilde{t}|\f$ where

\f[
\f{
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why this fixed this, or why it was an issue. Not a latex person.

Copy link
Member

Choose a reason for hiding this comment

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

If I remember right \f{ wil not center the formula, really strange that we get a warning here.

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

@birm you are my hero! This has needed to happen for way too long and I'm really appreciative that you just went and did it. I left some comments throughout (all of them should be easy I think!).

I noticed that in many places you added directories to the @file annotations. Do we want to add a 'general rule' about that? Where, e.g., all of the @file annotations start from under src/? e.g. @file mlpack/core.hpp or @file mlpack/bindings/cli/param.hpp, etc. Maybe that could be more consistent. Let me know what you think. 👍

Doxyfile Outdated Show resolved Hide resolved
doc/guide/hpt.hpp Outdated Show resolved Hide resolved
doc/guide/hpt.hpp Outdated Show resolved Hide resolved
doc/policies/elemtype.hpp Outdated Show resolved Hide resolved
doc/policies/kernels.hpp Outdated Show resolved Hide resolved
src/mlpack/methods/hoeffding_trees/hoeffding_tree.hpp Outdated Show resolved Hide resolved
src/mlpack/methods/hoeffding_trees/hoeffding_tree.hpp Outdated Show resolved Hide resolved
src/mlpack/methods/neighbor_search/neighbor_search.hpp Outdated Show resolved Hide resolved
birm and others added 2 commits May 12, 2020 20:34
@birm
Copy link
Member Author

birm commented May 13, 2020

Thanks both @zoq and @rcurtin for looking over this giant PR!
Regarding the \file paths, I just used the minimum unique one. It's probably reasonable to ask contributors to make this change to both new and old files as required, but maybe it's better to keep it consistent. I'm not sure, so open to thoughts on that.

@rcurtin
Copy link
Member

rcurtin commented May 13, 2020

Regarding the \file paths, I just used the minimum unique one. It's probably reasonable to ask contributors to make this change to both new and old files as required, but maybe it's better to keep it consistent. I'm not sure, so open to thoughts on that.

I thought it would be super tedious to use a full path, but, then I realized it's really easy to write a script to do it:

for i in `find ./ -iname '*.[hc]pp' -printf '%P\n'`; do sed -i 's|^ \* @file.*$| \* @file '$i'|' $i; done

(It could probably be done entirely with find, but, whatever.)

You can run that from src/ if you want the @files to be like @file mlpack/bindings/cli/end_program.hpp, or run from src/mlpack/ if you want the @files to be like @file bindings/cli/end_program.hpp. I don't think I have an opinion on which of those is better.

doc/policies/kernels.hpp Outdated Show resolved Hide resolved
@birm
Copy link
Member Author

birm commented May 13, 2020

the idea of having the full paths in \file looks good to me now that it's done. On a quick look, I didn't notice any cases where this made the line too long.

@zoq
Copy link
Member

zoq commented May 14, 2020

Let me rerun the failed ci jobs.

@zoq
Copy link
Member

zoq commented May 14, 2020

For some reason the job keeps failing, but not only for this PR.

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Files changed: 1,207 😮

Anyway, looks good to me. I'm not bothered by the randomly failing build (it doesn't seem related to this PR, at least). Thank you for putting in the time to get this all worked out! 👍

@birm
Copy link
Member Author

birm commented May 15, 2020

Let me pull in upstream changes... Now it's a 1209, which is cool because 1209 divides 92^2 - 1 (I was hoping for a more interesting fact about 1209 from wolfram...)

@rcurtin
Copy link
Member

rcurtin commented May 15, 2020

I tried Wikipedia; 1209 turns out to be the year Cambridge University was founded.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Wow, this is just great, no further from my side, thanks!

@birm
Copy link
Member Author

birm commented May 15, 2020

Any requests for commit cleanup? (Should I leave it be, squash some commits, squash all)

@birm birm merged commit c06eb3a into mlpack:master May 16, 2020
@birm birm mentioned this pull request May 18, 2020
kartikdutt18 added a commit to kartikdutt18/mlpack that referenced this pull request May 23, 2020
kartikdutt18 added a commit to kartikdutt18/mlpack that referenced this pull request May 23, 2020
@zoq zoq mentioned this pull request Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants