-
Notifications
You must be signed in to change notification settings - Fork 272
docs: initial platform improvements #483
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
base: main
Are you sure you want to change the base?
docs: initial platform improvements #483
Conversation
|
📚 Documentation Preview ✅ A preview of the documentation changes in this PR is available for maintainers at: Last updated: 2025-10-23 15:26 UTC |
jeremyshoemaker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a suggestion, but otherwise, it looks fine to me.
| contributor: jeremyshoemaker | ||
| --- | ||
|
|
||
| If we (Injection #9 Integrate a second simulator that runs Python >= 3.8) and we **(Injection #16 Integrate a second simulator that runs on CPU)** and we (Injection #17 Integrate a second simulator that runs on Windows) and we (Injection #19 Integrate a second simulator that does not require conda), then we achieve the desired effect (141 Monty is decoupled from a specific simulator implementation). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find these hard to read. Would it make sense to change these to links to the relevant pages and remove the "Injection #N" parts?
| If we (Injection #9 Integrate a second simulator that runs Python >= 3.8) and we **(Injection #16 Integrate a second simulator that runs on CPU)** and we (Injection #17 Integrate a second simulator that runs on Windows) and we (Injection #19 Integrate a second simulator that does not require conda), then we achieve the desired effect (141 Monty is decoupled from a specific simulator implementation). | |
| If we [integrate a second simulator that runs Python >= 3.8](TODO) and we **[integrate a second simulator that runs on CPU](TODO)** ... etc. |
If you do this, you could change the links to use Markdown's "reference links" feature where you only specify the URL once, since we have the list below under "Related Work".
A [link to injection 1][injection-1] and another [link to injection 1][injection-1]
[injection-1]: https://example.com/path/to/injection/1There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant. These are the names of nodes in the graph depicted in the image below. The sentence is a prose representation of the graph, intended to reinforce how the graph should be interpreted.
vkakerbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :) just two small comments on providing a bit more of our current knowledge and context
|
|
||
|  | ||
|
|
||
| ## Related work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be worth including a bit more detail on explorations we already did into simulators that meet those requirements. For example linking to this thread https://thousandbrains.discourse.group/t/the-next-monty-3d-environment-simulator/288
| --- | ||
| title: "Injection #17: Integrate a second simulator that runs on Windows." | ||
| tags: platform, simulator, environment | ||
| skills: python, mujoco |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The skill listed here is mujoco but none of the description mentions why. Maybe add a sentence saying that Mujoco seems to meet all these requirements
codeallthethingz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, there are a couple other fields that you may wish to add but neither are required:
improved-metric: https://github.com/codeallthethingz/tbp.monty/blob/f4fcfb8ebad35cf4a8cb919f559ce0a5d6ed31e2/docs/snippets/future-work-improved-metric.md
|
This has been open for a while, and the last comment was back in October. It looks like it has the approvals it needs. Is there some follow up needed, or can it get merged? |
|
@jeremyshoemaker I'll come back to it once I've wrapped up the configuration work. |
This pull request adds initial information on Platfrom improvements from the engineering future reality tree.
@codeallthethingz could you review and see if I added appropriate metadata for the anticipated future work widget?
@vkakerbeck and @jeremyshoemaker is the information content here sufficient for a future work page?