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

use features factory in notebooks #4269

Merged
merged 9 commits into from May 8, 2018

Conversation

vinx13
Copy link
Member

@vinx13 vinx13 commented May 1, 2018

[WIP]

" \n",
" gmm_est=GMM(num_components)\n",
" gmm_est.set_features(features)\n",
" gmm_est.set_features(features(X.T))\n",
Copy link
Member

Choose a reason for hiding this comment

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

can you use put here?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually we can't, GMM is a CDistrubition, where 'features' is not registered

Copy link
Member

Choose a reason for hiding this comment

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

ok!

"outputs": [],
"source": [
"def estimate_gmm(X, num_components):\n",
" # bring data into shogun representation (note that Shogun data is in column vector form, so transpose)\n",
" features=RealFeatures(X.T)\n",
Copy link
Member

Choose a reason for hiding this comment

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

for the sake of minimal changes, i would not delete this and do inline below, but rather replace the line with the factory

@@ -551,9 +527,7 @@
{
"cell_type": "code",
"execution_count": null,
"metadata": {
Copy link
Member

Choose a reason for hiding this comment

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

in order to avoid all these unrelated changes in the diff, could you first send a PR where do dont change anything, but rather just save the notebook, and then do a second Pr with your changes.
But only do it next time, this one is OK

@@ -590,7 +564,7 @@
" * pull model likelihood out of sample\n",
" * proper mixture model interface with classical distribution and EM interface (E,M methods)\n",
" \n",
" * set_coef dimensiosn check. DImension check in general\n",
" * put('m_coefficients', _) dimensiosn check. DImension check in general\n",
Copy link
Member

Choose a reason for hiding this comment

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

not sure about this

@@ -1,1078 +1,1041 @@
{
"cells": [
Copy link
Member

Choose a reason for hiding this comment

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

as you can see, this diff here is everything. Can you send a separate PR that just stores the nb in the new format and then update this one here to just have your changes? thx

@karlnapf
Copy link
Member

karlnapf commented May 8, 2018

Great!
This all looks good to me.
Before I merge, did you run all the notebooks? They run without errors?

@karlnapf karlnapf merged commit cceeb31 into shogun-toolbox:develop May 8, 2018
ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019
* Update notebook GMM
* Update KMeans notebook
* Update KernelDensity notebook
* Update Tapkee notebook
* Update Classification notebook
* Update SVM notebook
* Update Regression notebook
* Update PCA notebook
* Update ica notebooks
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

2 participants