-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add custom estimator for IP Insights algorithm #493
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
==========================================
+ Coverage 94.18% 94.24% +0.06%
==========================================
Files 58 59 +1
Lines 4400 4448 +48
==========================================
+ Hits 4144 4192 +48
Misses 256 256
Continue to review full report at Codecov.
|
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! just a small handful of nitpicky comments
src/sagemaker/amazon/ipinsights.py
Outdated
batch_metrics_publish_interval=None, epochs=None, learning_rate=None, | ||
num_ip_encoder_layers=None, random_negative_sampling_rate=None, | ||
shuffled_negative_sampling_rate=None, weight_decay=None, **kwargs): | ||
"""IP Insights is :class:`Estimator` used for unsupervised learning. |
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.
nitpick: change this to either
``IPInsights`` is a :class:`Estimator` used for unsupervised learning.
or something like
This estimator is for IP Insights, an algorithm for unsupervised learning.
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.
Updated to:
This estimator is for IP Insights, an unsupervised algorithm that learns usage patterns of IP addresses.
src/sagemaker/amazon/ipinsights.py
Outdated
weight_decay (float): Optional. Weight decay coefficient. Adds L2 regularization. | ||
**kwargs: base class keyword argument values. | ||
""" | ||
|
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.
nitpick: don't need this newline
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.
Done
src/sagemaker/amazon/ipinsights.py
Outdated
self.weight_decay = weight_decay | ||
|
||
def create_model(self, vpc_config_override=VPC_CONFIG_DEFAULT): | ||
"""Return a :class:`~sagemaker.amazon.IPInsightsModel`referencing the latest |
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.
this can go in a Returns
section after Args
. also, add a space after the last backtick
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.
Done
tests/unit/test_ipinsights.py
Outdated
@@ -0,0 +1,235 @@ | |||
|
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.
remove the newline
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.
Done
Issue #, if available:
Description of changes:
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.