-
Notifications
You must be signed in to change notification settings - Fork 245
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 QDT #442
Add QDT #442
Conversation
Hi Takuma-san, Sorry, I fixed some bugs in qdt.py. regards, |
@takuyamagata Thank you for making this contribution! Please let me take a look in this weekend. |
@takuyamagata Hi, Yamagata-san. The code looks good to me! Before merging your PR, can you fix the format error in CI? You should be able to fix this as follows:
It seems that there is something wrong in the unit test. But, I can follow up on this in a separate thread. Once the format check is passed, let me merge this PR. Thanks! |
@takuseno Thank you, Takuma-san. I have run the lint script. I hope it fixed the format error. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #442 +/- ##
=======================================
Coverage 91.74% 91.74%
=======================================
Files 122 122
Lines 8719 8719
=======================================
Hits 7999 7999
Misses 720 720 ☔ View full report in Codecov by Sentry. |
@takuyamagata Hi Yamagata-san. It seems that there are still some format issues. Can you check this error message and resolve it? |
@takuseno Hi Takuma-san, I have locally fixed most of the format errors, but one of the fixes causes an error in my environment. I need the following one line to make MuJoCo environments recognised in the script. I also manually add this to other scripts in reproductions/ to make it work. Could I include the line in qdt.py despite it causing a lint error? (Or is it just me have this issue??) |
Thank you for your work! d4rl is internally imported inside Line 418 in aa5ede9
Thus, you don't have to import it by yourself in the script. |
@takuyamagata There are mypy errors as follows:
Can you make sure that |
@takuseno I have fixed all format issues (including type check issues). Hope it is all good now. Regarding d4rl.gym_mujoco, I see why I get the error. I haven't set up other MuJoCo-based environments correctly, so d4rl.init.py fails to call 'import d4rl.gym_mujoco'. So it is my problem. :( |
Thank you for the changes! LGTM. |
In a follow-up commit, the script has been a little refactored: |
Hi Takuma-san,
I have implemented QDT. As QDT is not a model but data augmentation (a data-centric approach), it is currently implemented within reproductions/offline/qdt.py.
I am not 100% convinced that it is the right place. Please let me know if you have a better idea of where it should go or how it should be structured. I am happy to change it.
Also, I appreciate any suggestions to improve the code quality!
Thanks,
Taku