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

Added Documentation regarding issue #1878 #1961

Closed
wants to merge 20 commits into from
Closed

Added Documentation regarding issue #1878 #1961

wants to merge 20 commits into from

Conversation

kislayabhi
Copy link
Contributor

Added 'pca_notebook.ipynb' named python notebook in
doc/ipython-notebooks/pca
Implemented PCA on toy data for 2d to 1d and 3d to 2d projection.
Implemented Eigenfaces for data compression and face recognition
using att_face dataset.

Added 'pca_notebook.ipynb' named python notebook in
doc/ipython-notebooks/pca
Implemented PCA on toy data for 2d to 1d and 3d to 2d projection.
Implemented Eigenfaces for data compression and face recognition
using att_face dataset.
@vigsterkr
Copy link
Member

great! but please make sure that the output of the notebook is always clear, we don't want to store the generated output of the notebook in the repository.

@karlnapf
Copy link
Member

karlnapf commented Mar 8, 2014

Thanks!

One thing. Could you always post an nbviewer link to the current version of the notebook in the PR (with output)
The PR itself should not contain any output, so clear before you commit.

@karlnapf
Copy link
Member

karlnapf commented Mar 8, 2014

Data should be added to shogun-data (if open) and then the notebook should reference that.
Downloading manually is not a choice

@kislayabhi
Copy link
Contributor Author

@karlnapf , yeah!! I will amend those.

@kislayabhi
Copy link
Contributor Author

"cell_type": "markdown",
"metadata": {},
"source": [
"PCA finds a linear projection of high dimensional data into a lower dimensional subspace such that the variance is retained and least square reconstruction error is minimized."
Copy link
Member

Choose a reason for hiding this comment

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

We have a template for ipython notebooks, pls use the style suggested in there regarding headers, title, author list etc.

@karlnapf
Copy link
Member

karlnapf commented Mar 9, 2014

I really like where this is going, but there are currently many issues that need to be addressed.
Notebooks in general needs loads of love to look nice and convincing.
Again, pls have a look into other ones such as the GP to get some idea where we want to go with those notebooks

@karlnapf
Copy link
Member

karlnapf commented Mar 9, 2014

And thanks for the hard work :)

@vigsterkr
Copy link
Member

just one more thing: .ipynb_checkpoints directory or anything under it should not be added to repository

@kislayabhi
Copy link
Contributor Author

@karlnapf , @vigsterkr : I will get it done! Thanks for evaluating it. About the data, I sent a PR yesterday to the shogun-data with my dataset updated. Did I do something wrong there?

Again thanks. You have given me a direction to work on!! loving it :)

@karlnapf
Copy link
Member

karlnapf commented Mar 9, 2014

Great! Let us know when you have an updated version. You should put it under the same gist/nbviewer link.

@karlnapf karlnapf mentioned this pull request Mar 10, 2014
@kislayabhi
Copy link
Contributor Author

updated :) (http://nbviewer.ipython.org/gist/kislayabhi/9431770)
Phew, it took me some time.
Sorry for the delay

@karlnapf
Copy link
Member

"Here the best basis vectors are defined as"
Its not the best basis vectors, just the basis vectors

@karlnapf
Copy link
Member

"The optimal bias c is given"
Same here, bias is not optimal in the squared error, its just some bias

@karlnapf
Copy link
Member

"To find the best basis vectors B " Here we are talking about the best, so thats fine :)

@karlnapf
Copy link
Member

( i.e the basis vectors are mutually orthogonal and of unit length )

no whitespace after ( and before )

@karlnapf
Copy link
Member

EGD -- should be EVD I guess?

@karlnapf
Copy link
Member

"from matplotlib import pyplot" You can assume that our notebook server is started with
ipython notebook --pylab inline so there is not need to import anything. Also, all functions are loaded to the std namespace, so you can just do plot(...) instead of pylab.plot(...)

@karlnapf
Copy link
Member

Step 4: Calculate the eigenvectors and eigenvalues of the covariance matrix
Could you initialise PCA with explicitly using the constructors options and commenting on them? (not in the source code, but in notebook)

In fact, before you start with the PCA code, could you add a litlle paragraph on the CPCA class itself, quickly comment on how its initialised (see above, constructor parameters), and what type of data it accepts, what type of preprocessor it is, that it is a preprocessor in Shogun that is used in a certain way, etc? This is all Shogun specific, but its important to also document those points

@karlnapf
Copy link
Member

And in the above, also add link to the Shogun class list

@karlnapf
Copy link
Member

"Steps 5" there is an "s" too much

@karlnapf
Copy link
Member

"<matplotlib.text.Text at 0x3764bd0>"

Could you hide those type of outputs via _=plot(...)

@karlnapf
Copy link
Member

"Step6: Deriving the new data set"
What about "Projecting the data to its principal conponents", also a whitespace missing

@karlnapf
Copy link
Member

matrices (with D feature dimensions) supplied via apply_to_feature_matrix methods.This tranformation
#outputs the M-Dimensional approximation of all these input vectors and matrices (where M<=min(D,N)).

please dont do such things in comments, but rather in the notebook directly

@karlnapf
Copy link
Member

"preprocessor(from next example)" whitespace

@karlnapf
Copy link
Member

`````` ax.set_zlim(-30,30)please do_=ax.set_zlim(-30,30)``` to hide any python output for such plotting calls

@karlnapf
Copy link
Member

#E is automagically filled by setting target dimension = M. This is different from the 2d data example
#where we implemented this step manually.

Please write this in the notebook rather than in a comment

@karlnapf
Copy link
Member

"PCA Performance"
I just saw this, maybe this should be moved a bit to the top. I commented that its missing earlier

@karlnapf
Copy link
Member

Two-dimensional p*q grayscale should be "pq" (in latex), so no *

@karlnapf
Copy link
Member

"Step 1:Get" whitespace

@karlnapf
Copy link
Member

np.array etc can be array when you start the notebook with --pylab inline

@karlnapf
Copy link
Member

Euclidean distance - please add a link to the class documentation
http://www.shogun-toolbox.org/doc/en/latest/classshogun_1_1CEuclideanDistance.html

In fact, please do this for all classes you mention

@karlnapf
Copy link
Member

Wow, I have to say I am really impressed by this notebook! :)
It is very nicely written, and the examples are just amazing.
I hope you dont mind all my comments, it got so much better since the first iteration.
I think once my current issues are resolved, I will iterate/read once more and then we can merge this, and tell the world how great it is. Please add a comment in the NEWS file
-new PCA notebook by [yourname]
Great work!

@karlnapf
Copy link
Member

BTW I agree on your last point with the np.dot.
Also I hope your exams went well!

karlnapf and others added 6 commits March 18, 2014 01:44
Cleaning imports of several shogun classes in base, lib and machine
Added 'pca_notebook.ipynb' named python notebook in
doc/ipython-notebooks/pca
Implemented PCA on toy data for 2d to 1d and 3d to 2d projection.
Implemented Eigenfaces for data compression and face recognition
using att_face dataset.
@kislayabhi
Copy link
Contributor Author

Please refer to the following pull request.
pca ipython notebook revised 3rd iteration #2028
I somehow messed with my earlier forked repo :(

I have updated the nbviewer file

@kislayabhi kislayabhi closed this Apr 5, 2014
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

6 participants