-
Notifications
You must be signed in to change notification settings - Fork 119
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
Merge Diffuser Agent #301
Merge Diffuser Agent #301
Conversation
Hey Jiayi, I could you please kindly help me review the changes I made, and we can discuss design decision for dataset and evaluation integration? |
I will conduct a code review recently. Please check the items that failed in CI, namely Tests, and Lint. It would enhance the code quality. You can perform a self-check locally by running |
Sure.
2024年2月5日 16:32,Jiayi Zhou ***@***.***> 写道:
I will conduct a code review recently. Please check the items that failed in CI, namely Tests, and Lint. It would enhance the code quality. You can perform a self-check locally by running make lint and make test.
—
Reply to this email directly, view it on GitHub<#301 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJQOH3XOIIIYGTB6EEQR7V3YSECQXAVCNFSM6AAAAABCEEB6BWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRXGM4TCNZXHA>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
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 found a number of minor typos and I think we can make this PR better to correct them.
examples/collect_offline_data.py
Outdated
@@ -26,7 +26,7 @@ | |||
env_name = 'SafetyAntVelocity-v1' | |||
size = 1_000_000 | |||
agents = [ | |||
('PATH_TO_AGENT', 'epoch-500.pt', 1_000_000), | |||
('train/PPOLag-{SafetyAntVelocity-v1}/seed-000-2024-01-07-21-14-30', 'epoch-500.pt', 1_000_000), |
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.
Should we change it to more general form like 'PATH_TO_AGENT', and make 'train/PPOLag-{SafetyAntVelocity-v1}/seed-000-2024-01-07-21-14-30' as an example in docs?
examples/train_eval_diffuser.py
Outdated
@@ -0,0 +1,110 @@ | |||
# Copyright 2022-2024 OmniSafe Team. All Rights Reserved. |
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.
2024 OmniSafe Team, the same for others
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
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.
Missing # ==============================================================================
actor: DecisionDiffuserActor = agent._actor | ||
|
||
|
||
def cls_free_cond(actor: DecisionDiffuserActor) -> None: |
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 docstring should be google style.
""" | ||
condition on both state and cls free condition | ||
|
||
|
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.
Empty line
"""Decision Diffuser algorithm. | ||
|
||
References: | ||
- Something. |
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.
What is "Something" ?
+-------------------------+----------------------------------------------------+ | ||
| Things to log | Description | | ||
+=========================+====================================================+ | ||
| Loss/Loss | Loss of Diffusion and InvAR network | |
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.
Misaligned table.
Any clue with the test errors? I felt like we have wrong wandb version or something |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
==========================================
- Coverage 96.89% 91.20% -5.69%
==========================================
Files 138 148 +10
Lines 7000 7635 +635
==========================================
+ Hits 6782 6963 +181
- Misses 218 672 +454 ☔ View full report in Codecov by Sentry. |
Due to a prolonged lack of response, we have temporarily closed it. You are welcome to reopen it! |
Description
Implemented the decision diffuser. Since there will be quite a lot of breaking change to train-eval-loop, I am making an initial pull request to discuss the change in API.
Decision Diffuser.pdf
The main contributions are:
Motivation and Context
Initial task for Internship.
Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Checklist
Go over all the following points, and put an
x
in all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!
make format
. (required)make lint
. (required)make test
pass. (required)