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 factory of embed string features #4375

Merged

Conversation

vinx13
Copy link
Member

@vinx13 vinx13 commented Jul 16, 2018

add another string features factory that calls obtain_from_char internally to translate a string features to high order representation

@@ -106,6 +106,50 @@ namespace shogun
return result;
}

/** Create embed string features from string char features
Copy link
Member

Choose a reason for hiding this comment

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

"embedded"

@@ -106,6 +106,50 @@ namespace shogun
return result;
}

/** Create embed string features from string char features
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 user facing doc, so you need to make it a bit more user friendly

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.

Good apart from those docs

/** Create embed string features from string char features
/** Create embedded string features from string char features.
* The new features has the same alphabet as the original features. Data of
* the new features is obtained by calling `obtain_from_char` with the given
Copy link
Member

Choose a reason for hiding this comment

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

Much better. One final thing, could you make doxygen link the class method here?

@karlnapf karlnapf merged commit f8963c1 into shogun-toolbox:develop Jul 19, 2018
karlnapf pushed a commit to karlnapf/shogun that referenced this pull request Jul 20, 2018
ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants