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

cb_adf and cb_explore_adf: Setting --cb_type mtr as default #1838

Merged
merged 17 commits into from May 1, 2019

Conversation

Projects
None yet
3 participants
@marco-rossi29
Copy link
Collaborator

commented Apr 10, 2019

No description provided.

@marco-rossi29

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 10, 2019

@jackgerrits I discussed with @JohnLangford to make mtr the default cb_type. Some tests are failing since we change the default behavior. How do you suggest to proceed?

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

cb_algs.cc contains a default of dr and cb_explore_adf.cc contains a default of ips. Do we want to take this opportunity to normalize them all to mtr?

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

mtr does not exist for non-adf.

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

In that case should cb_explore_adf.cc be mtr and cb_algs.cc remain as dr?

@marco-rossi29 marco-rossi29 changed the title cb_adf: Setting --cb_type mtr as default cb_adf and cb_explore_adf: Setting --cb_type mtr as default Apr 12, 2019

@marco-rossi29 marco-rossi29 force-pushed the marco-rossi29/default_cb_type_mtr branch from 93e84e8 to bf9934a Apr 24, 2019

@marco-rossi29 marco-rossi29 force-pushed the marco-rossi29/default_cb_type_mtr branch from a6ae157 to e25bc79 Apr 24, 2019

@marco-rossi29 marco-rossi29 force-pushed the marco-rossi29/default_cb_type_mtr branch from e25bc79 to 53ac796 Apr 24, 2019

@marco-rossi29

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 25, 2019

I fixed all the errors in RunTests. However, now Travis is failing on Java Tests. @JohnLangford Is the a simple flag for the Java tests?

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I don't think so.

@marco-rossi29 marco-rossi29 force-pushed the marco-rossi29/default_cb_type_mtr branch 3 times, most recently from f552b1a to 53ac796 Apr 25, 2019

@marco-rossi29 marco-rossi29 force-pushed the marco-rossi29/default_cb_type_mtr branch from 53ac796 to 822158d Apr 29, 2019

@marco-rossi29 marco-rossi29 reopened this Apr 29, 2019

@marco-rossi29

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2019

This PR is blocked on LGTM failing Java analysis for VW master (I've on purpose placed a dummy change in the help section of cb_explore_adf.cc)

@marco-rossi29 marco-rossi29 force-pushed the marco-rossi29/default_cb_type_mtr branch from 9208659 to f552b1a Apr 29, 2019

@marco-rossi29 marco-rossi29 force-pushed the marco-rossi29/default_cb_type_mtr branch from f552b1a to c0f7471 Apr 29, 2019

jackgerrits added some commits Apr 29, 2019

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@marco-rossi29 tests are now passing

jackgerrits and others added some commits Apr 30, 2019

@marco-rossi29

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2019

This is ready to be squashed-merged!

@jackgerrits jackgerrits merged commit f96e5fb into master May 1, 2019

8 of 9 checks passed

LGTM analysis: Java Analysis failed (could not build the merge commit and the base commit (f159ff6))
Details
LGTM analysis: C# No new or fixed alerts
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 72.956%
Details
@jackgerrits

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Thanks Marco!

@marco-rossi29 marco-rossi29 deleted the marco-rossi29/default_cb_type_mtr branch May 2, 2019

jackgerrits added a commit to jackgerrits/vowpal_wabbit that referenced this pull request May 15, 2019

cb_adf and cb_explore_adf: Setting --cb_type mtr as default (VowpalWa…
…bbit#1838)

* cb_adf: Setting --cb_type mtr as default

* cb_explore_adf.cc: default cb_type mtr

* Removed dm from cb_type help, since dm is not supported

* Fixed test 84

* cb_adf: fixed test 87

* cb_explore_adf: Fixed test 129

* cb_explore_adf: Fixed test 138

* cb_explore_adf: Fixed test 158

* Fixing Java tests errors

* Update tests

* Precision

* Precision

* ordering

* precision

* Fix ips->mtr in comment + adding spaces in cmd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.