Skip to content

NormalizeMinMax Multicolumn example #4644

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

Merged
merged 6 commits into from
Feb 5, 2020

Conversation

jwood803
Copy link
Contributor

@jwood803 jwood803 commented Jan 11, 2020

Update for #3436 for the NormalizeMinMax transform.

@sfilipi Took a shot at adding a multicolumn sample. If this is on the right track, I can help with some of the others.

@jwood803 jwood803 requested a review from a team as a code owner January 11, 2020 12:22
@antoniovs1029 antoniovs1029 requested a review from natke January 14, 2020 18:06
Copy link
Contributor

@natke natke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few corrects and comments. Please feel free to reach out if anything is unclear

// 0.0000, 0.0000, 0.3333, 0.0000 0.6667, 1.0000, 1.0000
// -0.5000, -0.5000, -0.3333, 1.0000 1.0000, 0.0000, 0.5000

// Let's get transformation parameters. Since we work with only one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we work with only one -> Since there is only one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have two columns here? Features and Features2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the wording of the comment, but I'll need to see how the formula changes since there are two columns instead of one.

Copy link
Contributor Author

@jwood803 jwood803 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natke Thanks for the review! Made most of the changes, but I would need to update the last output for the formula since there are now two columns.

@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #4644 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4644   +/-   ##
=======================================
  Coverage   75.84%   75.84%           
=======================================
  Files         947      947           
  Lines      172170   172170           
  Branches    18576    18576           
=======================================
  Hits       130585   130585           
  Misses      36416    36416           
  Partials     5169     5169
Flag Coverage Δ
#Debug 75.84% <0%> (ø) ⬆️
#production 71.43% <0%> (ø) ⬆️
#test 90.7% <0%> (ø) ⬆️

@najeeb-kazmi
Copy link
Member

@jwood803 please pull master, so that you have this commit. Then the tests that fail randomly should pass, so we can merge your PR.

@najeeb-kazmi
Copy link
Member

@natke does this look good to you? If so, let's try to merge this before release so that the example shows up in the docs.

@antoniovs1029 antoniovs1029 merged commit d2ff3e3 into dotnet:master Feb 5, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants