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

Created cookbook for passing vectors/matrix to features object #4142

Conversation

yashdusing
Copy link
Contributor

Feature/vector matrix cookbook
Issue #4105

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the patch. This is really needed.
I did some more heavy reviewing as it is crucial to get this right.


#![create_matrix]
RealMatrix real_matrix(2,3)
real_matrix[0,0]= Math:random(0, 5)
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer if you put explicit values in here, 0,1,2,3,4,....

And then later in the notebook, you can extract feature vectors/matrices get_feature_vector(0) etc

#![create_features]
RealFeatures features(real_vector)
#![create_features]

Copy link
Member

Choose a reason for hiding this comment

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

add some extraction illustration, i.e. how to get the vectors back from Shogun

#![create_vector]
RealVector real_vector(5)
real_vector[0]= Math:random(0, 5)
real_vector[1]= Math:random(0, 5)
Copy link
Member

Choose a reason for hiding this comment

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

same comments as above

@@ -0,0 +1,33 @@
============================
Copy link
Member

Choose a reason for hiding this comment

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

Filename should be just "features"

@@ -0,0 +1,33 @@
============================
Passing features from vector
Copy link
Member

Choose a reason for hiding this comment

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

Make this: "Passing data to Shogun"

NOTE
----

It is important to note that conventionally, data is stored in a row-major fashion, meaning two row vector of size 3.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: "Note that data is stored in column major format, i.e. each column of the matrix corresponds to an observation / feature vector, where each vector consists of a number of variables that is equal to the number of rows of the matrix."
I.e. add the wikipedia link for explanation and for the feature vector

Copy link
Member

Choose a reason for hiding this comment

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

And remove the other sentences

It is important to note that conventionally, data is stored in a row-major fashion, meaning two row vector of size 3.
However, in Shogun, this will be column vectors of dimension 2. This is beacuse each data sample is stored in a column-major fashion,
meaning each column here corresponds to an individual sample and each row in it to an atribute like BMI, Glucose concentration etc.
Also, it is essential to map the data types of the features object & vector i.e one cannot pass a vector of integers to a method that expects floats
Copy link
Member

Choose a reason for hiding this comment

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

This is a good point. Maybe it should be phrased "Note that features are type-safe, e.g. you can only pass 64 bit floating point numbers to RealFeatures.

----------
References
----------
:wiki:`Feature (machine learning)`
Copy link
Member

Choose a reason for hiding this comment

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

good, also put the column major thing

@@ -0,0 +1,16 @@
Math:init_random(1)

Copy link
Member

Choose a reason for hiding this comment

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

I in fact suggest to remove the vector example

@yashdusing
Copy link
Contributor Author

Thank you for the feedback ! I will try to solve this as soon as possible.
I'm still new to github & I couldn't find how to update existing PR . Will look into it.
Thanks once again !

@karlnapf
Copy link
Member

karlnapf commented Feb 3, 2018

@stale
Copy link

stale bot commented Feb 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 26, 2020
@stale
Copy link

stale bot commented Mar 4, 2020

This issue is now being closed due to a lack of activity. Feel free to reopen it.

@stale stale bot closed this Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants