-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add a bayesian gaussian mixture model transformer #329
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.
Left a couple of comments but it's looking good so far!
rdt/transformers/numerical.py
Outdated
one_hot[np.arange(data.shape[0]), discrete_column] = 1.0 | ||
data = np.concatenate([data[continuous_name][:, None], one_hot], axis=1) | ||
print(data.shape) | ||
one_hot = np.zeros(shape=(data.shape[1], self._number_of_modes)) |
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 are we changing the indexing?
Codecov Report
@@ Coverage Diff @@
## master #329 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 12 12
Lines 829 1010 +181
==========================================
+ Hits 829 1010 +181
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.
This is looking pretty good. I just had a few questions.
The main thing I am wondering is if the NullTransformer
should just be used directly. I believe that you are using the NumericalTransformer
super methods for that reason, and since eventually the NullTransformer
will be pulled out, maybe it makes sense to just use it directly and not use the NumericalTransformer
at all
@amontanez24 The |
I don't think there is a reason. We just didn't add it when we created those parameters |
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 is ready!
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 addressing the feedback @fealho.
This looks good to go, now!
Resolve #183.