-
Notifications
You must be signed in to change notification settings - Fork 244
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
added wandb logger and extended when to log #379
Conversation
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.
@claudius-kienle Thank you for your contribution! This is awesome! I have some minor comments that I'd like you to address before we merge this. Please take a look. Thanks again!
@takuseno Thank you for the review. All your comments should now be included. |
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.
Thanks for the change! It looks good to me. I have one minor request that I want you to address!
d3rlpy/constants.py
Outdated
@@ -47,3 +47,8 @@ class ActionSpace(Enum): | |||
class PositionEncodingType(Enum): | |||
SIMPLE = "simple" | |||
GLOBAL = "global" | |||
|
|||
|
|||
class LoggingStrategyEnum(Enum): |
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.
Could you rename this to LoggingStrategy
?
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.
Of course. I just changed it.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #379 +/- ##
==========================================
- Coverage 91.96% 91.61% -0.36%
==========================================
Files 113 114 +1
Lines 8012 8047 +35
==========================================
+ Hits 7368 7372 +4
- Misses 644 675 +31 ☔ View full report in Codecov by Sentry. |
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.
Thanks! LGTM. I see formattting errors, but I'll follow up.
I added a wandb logger adapter and also added to new parameters to customize when to log.
This will close #362