-
Notifications
You must be signed in to change notification settings - Fork 713
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
addeed policy learning to readme #440
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.
Mostly looks fine, a few minor comments/suggestions
README.md
Outdated
``` | ||
|
||
|
||
![image](notebooks/images/policy_tree.png) |
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.
Why put the image under notebooks?
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.
Not that it's super important, but changes to the notebooks directory trigger the notebook tests to run, which should not be necessary for this change.
(However, if you create a new directory (/images/
, say) that we want to allow changes to without running tests you'll need to add a line for it here; the change to the pipelines file will itself force all tests to run, but then subsequent changes to that directory will be free)
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.
that's were all our images are
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'll move then all readme images there. That should still trigger notebook tests, as I'll need to move images out of the notebooks folder
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.
Created a new image folder and moved only that image there. Added that image fodler to azure-pielines
from sklearn.ensemble import RandomForestRegressor | ||
|
||
# fit a single binary decision tree policy | ||
policy = DRPolicyTree(max_depth=1, min_impurity_decrease=0.01, honest=True) |
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.
Are the variations from the default init args important for this example? Would a user understand why to set these particular values?
Or in other words, if I'm going to copy and paste this to start playing around with it, are these reasonable settings, and if so why are they not already the defaults?
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.
not sure. migth be reasonable. Was more showing some capabilities of what is parameterizable
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.
If the defaults are reasonable, and these arguments are also reasonable, and there's no particular reason to assume one is better than the other, then that's fine.
Otherwise, I'd try to make sure that whatever you put here will be a good start for someone to copy and paste, rather than show off functionality that's more advanced but requires more knowledge to use well.
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.
yeap I think this is good and along these lines
policy.feature_importances_ | ||
|
||
# fit a binary decision forest | ||
policy = DRPolicyForest(max_depth=1, min_impurity_decrease=0.01, honest=True) |
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.
Same question about defaults here.
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.
yeah same. seems good parameters the users might want to play with.
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!
No description provided.