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

Add SUMO-RL as example project in the docs #257

Merged
merged 6 commits into from
Dec 13, 2020

Conversation

LucasAlegre
Copy link
Contributor

@LucasAlegre LucasAlegre commented Dec 9, 2020

Description

Added SUMO-RL (https://github.com/LucasAlegre/sumo-rl) as example project in the docs

Motivation and Context

  • I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)

Checklist:

  • I've read the CONTRIBUTION guide (required)
  • I have updated the changelog accordingly (required).
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.
  • I have reformatted the code using make format (required)
  • I have checked the codestyle using make check-codestyle and make lint (required)
  • I have ensured make pytest and make type both pass. (required)
  • I have checked that the documentation builds using make doc (required)

Note: You can run most of the checks using make commit-checks.

Note: we are using a maximum length of 127 characters per line

@araffin araffin assigned araffin and unassigned araffin Dec 9, 2020
@araffin araffin self-requested a review December 9, 2020 20:23
@araffin
Copy link
Member

araffin commented Dec 9, 2020

Hello,

Thank you for your PR, please don't forget to do all the required steps in the checklist ;)

@araffin
Copy link
Member

araffin commented Dec 13, 2020

Thank you for your PR, please don't forget to do all the required steps in the checklist ;)

If you check an item in the checklist, you must DO it first ;)

@@ -53,6 +53,8 @@ Documentation:
- Fix ``clip_range`` docstring
- Fix duplicated parameter in ``EvalCallback`` docstring (thanks @tfederico)
- Added example of learning rate schedule
- Added SUMO-RL as example project (@LucasAlegre)
- Fix docstring of AtariWrapper which was not inside of __init__ method (@LucasAlegre)
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this, docstrings are right after the constructor for the automatic documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it was giving an error when running "make-doc". Now it is like all other classes.

Copy link
Member

Choose a reason for hiding this comment

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

That should not be the case, the doc is building at least with continuous integration, see https://github.com/DLR-RM/stable-baselines3/runs/1533347883

See current doc too: https://stable-baselines3.readthedocs.io/en/master/common/atari_wrappers.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error I found before was because it was written "frame_skip::" instead of "frame_skip:" (with only one ":").
Now I fixed the docs of all the other classes in the file too.

@LucasAlegre
Copy link
Contributor Author

Thank you for your PR, please don't forget to do all the required steps in the checklist ;)

If you check an item in the checklist, you must DO it first ;)

Sorry! I thought some of them were not necessary as I had not changed code files.

Now I believe everything is passing! :)

Copy link
Member

@araffin araffin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks =)

@LucasAlegre
Copy link
Contributor Author

LGTM, thanks =)

Thank you! And sorry for the trouble with the checklist.

By the way, I'm working on a PyTorch version of Model-Based Policy Optimization (MBPO) reusing SAC from sb3.
Perhaps I can add it to stable-baselines3-contrib when I'm done.

Cheers

@araffin araffin merged commit b8c72a5 into DLR-RM:master Dec 13, 2020
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.

2 participants