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

[doc] Provide an inventory of our environment variables #19045

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Mar 21, 2023

Towards #19044.

Amends #18819.


image


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: none This pull request should not be mentioned in the release notes labels Mar 21, 2023
@jwnimmer-tri
Copy link
Collaborator Author

+@hongkai-dai for feature review of gurobi_solver.h please.

+@ggould-tri for feature review of the remainder & platform review per schedule, please.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees ggould-tri(platform),hongkai-dai (waiting on @hongkai-dai and @jwnimmer-tri)

a discussion (no related file):
Is this the right place for the documentation? In our current implementation aiui this will not be linked or linkable from the pydrake docs. This might be better off in the Jekyll-based doc tree rather than in language-specific API docs.



doc/doxygen_cxx/doxygen.h line 217 at r1 (raw file):

 @defgroup environment_variables Environment Variables

 This sections provides an inventory of environment variables relevant to Drake.

typo

Suggestion:

section

doc/doxygen_cxx/doxygen.h line 217 at r1 (raw file):

 @defgroup environment_variables Environment Variables

 This sections provides an inventory of environment variables relevant to Drake.

minor: Alternatively, describe the content not the documentation?

Suggestion:

Drake and its solvers and visualizers use several environment variables:

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignees ggould-tri(platform),hongkai-dai (waiting on @ggould-tri and @hongkai-dai)

a discussion (no related file):

Previously, ggould-tri wrote…

Is this the right place for the documentation? In our current implementation aiui this will not be linked or linkable from the pydrake docs. This might be better off in the Jekyll-based doc tree rather than in language-specific API docs.

If we wanted to make a hyperlink from pydrake API docs to here, it's no more difficult to link to doxygen than it is to jekyll. In either case, we just provide a (relative) URL; in neither case can we auto-link it with a short glyph.



doc/doxygen_cxx/doxygen.h line 217 at r1 (raw file):

Previously, ggould-tri wrote…

minor: Alternatively, describe the content not the documentation?

That's a little better in situ, but I think worse in the https://drake.mit.edu/doxygen_cxx/group__technical__notes.html table of contents.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai (waiting on @hongkai-dai)

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai (waiting on @hongkai-dai and @jwnimmer-tri)

a discussion (no related file):
Working

I realized that we should mention OMP_NUM_THREADS in this list as well.

\CC @calderpg-tri FYI


Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: LGTM missing from assignee hongkai-dai (waiting on @ggould-tri and @hongkai-dai)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I realized that we should mention OMP_NUM_THREADS in this list as well.

\CC @calderpg-tri FYI

Done.


Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee hongkai-dai (waiting on @hongkai-dai)

@jwnimmer-tri
Copy link
Collaborator Author

To keep this moving, I'm extracting the Gurobi docs to a follow-up PR.

-@hongkai-dai +(status: single reviewer ok)

@jwnimmer-tri jwnimmer-tri added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Mar 22, 2023
@jwnimmer-tri jwnimmer-tri merged commit 2d8aa72 into RobotLocomotion:master Mar 22, 2023
@jwnimmer-tri jwnimmer-tri deleted the doc-environ branch March 22, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants