-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Hash Transform API that takes in advanced options. #4443
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 #4443 +/- ##
=========================================
Coverage ? 74.71%
=========================================
Files ? 906
Lines ? 159289
Branches ? 17145
=========================================
Hits ? 119011
Misses ? 35476
Partials ? 4802
|
src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
Outdated
Show resolved
Hide resolved
1561272
to
3c60f09
Compare
Do you want to add a new functional test that uses the new advanced options? That way we ensure the actual API works as users will use it. #Resolved |
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 think this new API looks good to me. It should be ready to go in when we have functional tests covering the new API.
06a46e4
to
6b8a35f
Compare
@eerhardt I have added the functional test you asked for (even though I had a unit-test called HashWorkout that exercised this routine). Since the API looks good to you and is ready to go so I will check-in this PR (because I need to provide the nuget today) but if you have more comments please make here and I will include them in a separate PR. |
fixes #4422
Needed by a 1P customer.