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

Drop jinja2 in trained model serialization tests #4307

Merged
merged 1 commit into from Jun 5, 2018

Conversation

micmn
Copy link
Contributor

@micmn micmn commented May 25, 2018

It may need some cleanup but this should do it.

@karlnapf #3537

@vigsterkr
Copy link
Member

@micmn from appveyor:
[00:49:40] 15>c:\projects\shogun\tests\unit\base\trained_model_serialization_unittest.cc(127): fatal error C1083: Cannot open include file: 'trained_model_serialization_unittest.h': No such file or directory [C:\projects\shogun\build\tests\unit\shogun-unit-test.vcxproj]

SG_REF(train_labels)
}

bool serialize_machine(
Copy link
Member

Choose a reason for hiding this comment

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

shall we put this into a helper, this kind of stuff is used in multiple places in the tests

return load_success && (delete_success == 0);
}

CDenseFeatures<float64_t> *train_feats, *test_feats;
Copy link
Member

Choose a reason for hiding this comment

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

could we template the features as well?

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.

This is way better than the generated c++ code! Let's get it in! :)

@shubham808
Copy link
Contributor

@micmn I need to extend this for pausing tests so i am happy to complete this for u :)

@karlnapf
Copy link
Member

karlnapf commented Jun 1, 2018

@micmn thx for the patch. We will cherry pick your commit and move on from there if you dont mind?

@micmn
Copy link
Contributor Author

micmn commented Jun 1, 2018

Yes, of course :)

@karlnapf
Copy link
Member

karlnapf commented Jun 4, 2018

@micmn we have trouble fixing this error. Would you mind having a look? :)

@micmn micmn force-pushed the feature/drop-jinja branch 2 times, most recently from 1961292 to 7dac419 Compare June 4, 2018 17:18
@micmn
Copy link
Contributor Author

micmn commented Jun 5, 2018

green light!

hello Microsoft :o

@karlnapf
Copy link
Member

karlnapf commented Jun 5, 2018

Nice! Thanks!

@karlnapf karlnapf merged commit 94c093b into shogun-toolbox:develop Jun 5, 2018
@micmn micmn deleted the feature/drop-jinja branch June 5, 2018 11:16
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

4 participants