Replacing undocumented python example for Sparse Euclidian Distance with a meta-example #4945
Conversation
|
that doesn't matter... plz read up on git and github. there's absolutely no reason the close and open another PR.... except the case of a forced pushed branch while PR is being close.. in that case you cannot open a PR on github for some reason. other than that you should be able to have one PR open for a specific task... |
|
the linux tests are failing while trying to execute the meta example this PR adds. plz try to debug locally what's happening |
| #![create_features] | ||
|
|
||
| #![create_instance] | ||
| Distance d = distance("SparseEuclideanDistance", lhs=features_a, rhs=features_a) |
There was a problem hiding this comment.
as the documentation stats SparseEuclideanDistance is expecting SparseFeatures.... imo the features you are reading above are not SparseFeatures but DenseFeatures.
plz make sure that you compile and test your changes before sending in a PR
There was a problem hiding this comment.
In all the other examples(.sg.in files), features_a and features_b are defined as variables of the Features class, and they directly become DenseFeatures by calling the constructor of the CDenseFeatures class.
How should we ensure that the features are sparse and the constructor of the CSparseFeatures class is called?
There was a problem hiding this comment.
none of the examples are actually using methods that requires SparseFeatures... for that actually you'll need to patch factory.h to be able to read sparse format (say libsvmfile) and create a SparseFeature from it.
There was a problem hiding this comment.
That might be a bit beyond an entrance task so might be worth starting an issue and then rebase this when it’s done?
There was a problem hiding this comment.
Started new issue #4947
|
i think you haven't tested this code locally... |
|
I added 2 functions to the translate.TranslationFailure: 'Failed to obtain include path for Csparse_features or sparse_features or Csparse_features or sparse_features' |
| import shogun as sg | ||
|
|
||
| feats_train=SparseRealFeatures(sg.libsvm_file(train_fname)) | ||
| feats_test=SparseRealFeatures(sg.libsvm_file(test_fname)) |
There was a problem hiding this comment.
Why don’t you use this parser and files?
There was a problem hiding this comment.
Won't I have to add functions to the factory.h file nevertheless? so that it can be translated?
src/shogun/util/factory.h
Outdated
| @@ -130,6 +131,41 @@ namespace shogun | |||
| return result; | |||
| } | |||
|
|
|||
| template <class T> | |||
| std::shared_ptr<Features> sparse_features(SGMatrix<T> mat) | |||
There was a problem hiding this comment.
exposing the SparseMatrix ctor would be good as well, but as for factory we would like to have this under one common function, namely features.
src/shogun/util/factory.h
Outdated
| std::shared_ptr<Features> sparse_features(std::shared_ptr<File> file, EPrimitiveType primitive_type = PT_FLOAT64) | ||
| { | ||
| require(file, "No file provided."); | ||
| std::shared_ptr<Features> result = nullptr; |
There was a problem hiding this comment.
again as said above this should be within features factory function.
for that to be able to work you'll have to do a bit of dispatching, i.e. to detect the type of File object being passed (csv or libsvmfile) and based on that create the right feature type object (dense or sparse). i'd use type_index(typeid()) to detect the type of file that has been passed over, but you can even use get_name() and do a string comparison.
Hephaestus12
force-pushed
the
feature/metaExample
branch
from
7de1a06 to
55dbd89
Compare
|
@vigsterkr Is this okay? |
src/shogun/util/factory.h
Outdated
| @@ -106,25 +107,51 @@ namespace shogun | |||
| require(file, "No file provided."); | |||
| std::shared_ptr<Features> result = nullptr; | |||
|
|
|||
| switch (primitive_type) | |||
| if(strcmp( file->get_name(), "LibSVMFile") == 0) | |||
There was a problem hiding this comment.
@vigsterkr I guess this works, just not very maintainable... What are the other sparse data file format available?
There was a problem hiding this comment.
could you do something like std::type_index(typeid(*file)) == std::type_index(typeid(LibSVMFile))?
Hephaestus12
force-pushed
the
feature/metaExample
branch
from
55dbd89 to
99a0f1e
Compare
|
Is this fine? @gf712 @vigsterkr |
does it work locally? |
A few unit tests failed. However they are failing in the original code(before making the changes) as well. |
|
unit tests only cover c++ code, you need to build, run, and test the meta examples. Check the examples readme for that |
|
Could you trigger the CI please? It seems like the previous failures were due to the alignment bug, which is unrelated to this and has been fixed. If it passes CI now we can merge! |
…ith a meta-example
Hephaestus12
force-pushed
the
feature/metaExample
branch
from
99a0f1e to
3e0f2d0
Compare
|
Is this fine? |
|
@vigsterkr is this fine? |
|
yeah lets squash this one! i suppose the Accelerate fail is totally unrelated...right? |
yup, that's just the cblas header issue |
Removed
.pyfile.Added
.sg.infile.Compiled successfully on local machine.
#3555 #3000 #4942
Sorry for creating a new PR, had deleted the old branch by mistake.