Skip to content
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 GaussianMixture #169

Merged
merged 45 commits into from Jul 15, 2019
Merged

Add GaussianMixture #169

merged 45 commits into from Jul 15, 2019

Conversation

xadupre
Copy link
Collaborator

@xadupre xadupre commented Jun 6, 2019

No description provided.

@xadupre xadupre changed the title [WIP] Add GaussianMixture Add GaussianMixture Jun 10, 2019
n_features = X.type.shape[1]
n_components = op.means_.shape[0]

# def _estimate_weighted_log_prob(self, X):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why commented code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To remember where I found the implementation in scikit-learn.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to have comments instead. That would make it clear to anyone reading the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I did (line 29). Does it need more?

# self._estimate_log_prob(X) + self._estimate_log_weights()
log_weights = np.log(op.weights_) # self._estimate_log_weights()

# self._estimate_log_prob(X)
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented code again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same reason.

op.precisions_cholesky_, op.covariance_type, n_features)

if op.covariance_type == 'full':
# shape(op.means_) = (n_components, n_features)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a lot of commented code in this file, could you clean it up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to let it, it is how it is implemented in scikit-learn, I can add a new comment to specify it comes from sklearn.

def calculate_gaussian_mixture_output_shapes(operator):
check_input_and_output_numbers(operator, input_count_range=1,
output_count_range=2)
check_input_and_output_types(operator, good_input_types=[FloatTensorType])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is int not allowed as an input type? Scikit allows int features.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hesitate. Statistically, it makes no sense to fix a gaussian mixture on integer data as it cannot be gaussian. I'll fix it.

@xadupre xadupre merged commit 111e49d into onnx:master Jul 15, 2019
@xadupre xadupre deleted the gau branch November 14, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants