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.

Sign up for GitHub

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

Jump to bottom

Fix to save the state of FTRL models #1912

Merged
merged 16 commits into from Jun 5, 2019
Merged

Fix to save the state of FTRL models #1912

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5783622
first version of the KT algorithm
bremen79 Mar 6, 2019
07a7e04
changed from 'kt' to 'approximate cocob' and implemented normalizatio…
bremen79 Mar 13, 2019
9430ede
variant that works with squared loss
bremen79 Apr 24, 2019
9fba67d
fixed all bugs: works great in binary classification, slightly worse …
bremen79 May 8, 2019
83633b8
cleaned version, no bias used
bremen79 Jun 3, 2019
d31cef3
bias and fix bug
bremen79 Jun 3, 2019
49bccd5
another bug fix
bremen79 Jun 3, 2019
12f6a84
removed bias and added default params for logistic
bremen79 Jun 4, 2019
2591f6b
prediction is now stateless
bremen79 Jun 4, 2019
7540eca
added comments
bremen79 Jun 4, 2019
048c0f7
Merge branch 'master' into coin_pr_version
bremen79 Jun 4, 2019
a8bc3a5
added tests
bremen79 Jun 4, 2019
b19a82e
fix to ftrl state saving
bremen79 Jun 5, 2019
c13457d
merge
bremen79 Jun 5, 2019
14ae4ae
moved ftrl_size to a parameter of save_load_online_state
bremen79 Jun 5, 2019
39512b8
Merge branch 'master' into fix_save_ftrl
JohnLangford Jun 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion test/save_resume_test.py
View file
Open in desktop
Expand Up @@ -153,8 +153,9 @@ def do_test(filename, args, verbose=None, repeat_args=None, known_failure=False)
errors += do_test(filename, '--loss_function logistic --link logistic')
errors += do_test(filename, '--nn 2')
errors += do_test(filename, '--binary')
errors += do_test(filename, '--ftrl', known_failure=True)
errors += do_test(filename, '--ftrl')
errors += do_test(filename, '--pistol', known_failure=True)
Copy link
Member

Choose a reason for hiding this comment

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

Are pistol and coin still in known failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. As I write above, the data structure is saved correctly, but the weights seem to be different if trained continuously or trained, saved, resumed. The difference does not happen immediately, but after some samples. This is why I suspect numerical issues.

Copy link
Member

Choose a reason for hiding this comment

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

This violates my understanding of computers so I suspect there is something that we're missing. I probably won't be able to debug before the next release, but it should get done...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying to debug this issue a bit more

errors += do_test(filename, '--coin', known_failure=True)

# this one also fails but pollutes output
#errors += do_test(filename, '--ksvm', known_failure=True)
Expand Down
5 changes: 4 additions & 1 deletion vowpalwabbit/ftrl.cc
View file
Open in desktop
Expand Up @@ -151,7 +151,7 @@ void inner_update_pistol_state_and_predict(update_data& d, float x, float& wref)

float squared_theta = w[W_ZT] * w[W_ZT];
float tmp = 1.f / (d.ftrl_alpha * w[W_MX] * (w[W_G2] + w[W_MX]));
w[W_XT] = sqrt(w[W_G2]) * d.ftrl_beta * w[W_ZT] * correctedExp(squared_theta / 2 * tmp) * tmp;
w[W_XT] = sqrt(w[W_G2]) * d.ftrl_beta * w[W_ZT] * correctedExp(squared_theta / 2.f * tmp) * tmp;

d.predict += w[W_XT] * x;
}
Expand Down Expand Up @@ -389,18 +389,21 @@ base_learner* ftrl_setup(options_i& options, vw& all)
else
learn_ptr = learn_proximal<false>;
all.weights.stride_shift(2); // NOTE: for more parameter storage
all.ftrl_size = 3;
}
else if (pistol)
{
algorithm_name = "PiSTOL";
learn_ptr = learn_pistol;
all.weights.stride_shift(2); // NOTE: for more parameter storage
all.ftrl_size = 4;
}
else if (coin)
{
algorithm_name = "Coin Betting";
learn_ptr = learn_cb;
all.weights.stride_shift(3); // NOTE: for more parameter storage
all.ftrl_size = 6;
}

b->data.ftrl_alpha = b->ftrl_alpha;
Expand Down
20 changes: 17 additions & 3 deletions vowpalwabbit/gd.cc
View file
Open in desktop
Expand Up @@ -786,8 +786,10 @@ void save_load_online_state(vw& all, io_buf& model_file, bool read, bool text, g
if (i >= length)
THROW("Model content is corrupted, weight vector index " << i << " must be less than total vector length "
<< length);
weight buff[4] = {0, 0, 0, 0};
if (g == NULL || (!g->adaptive && !g->normalized))
weight buff[8] = {0, 0, 0, 0, 0, 0, 0, 0};
if (all.ftrl_size>0)
brw += model_file.bin_read_fixed((char*)buff, sizeof(buff[0]) * all.ftrl_size, "");
else if (g == NULL || (!g->adaptive && !g->normalized))
brw += model_file.bin_read_fixed((char*)buff, sizeof(buff[0]), "");
else if ((g->adaptive && !g->normalized) || (!g->adaptive && g->normalized))
brw += model_file.bin_read_fixed((char*)buff, sizeof(buff[0]) * 2, "");
Expand All @@ -812,7 +814,19 @@ void save_load_online_state(vw& all, io_buf& model_file, bool read, bool text, g
else
brw = bin_text_write_fixed(model_file, (char*)&i, sizeof(i), msg, text);

if (g == nullptr || (!g->adaptive && !g->normalized))
if (all.ftrl_size==3) {
msg << ":" << *v << " " << (&(*v))[1] << " " << (&(*v))[2] << "\n";
brw += bin_text_write_fixed(model_file, (char*)&(*v), 3 * sizeof(*v), msg, text);
}
else if (all.ftrl_size==4) {
msg << ":" << *v << " " << (&(*v))[1] << " " << (&(*v))[2] << " " << (&(*v))[3] << "\n";
brw += bin_text_write_fixed(model_file, (char*)&(*v), 4 * sizeof(*v), msg, text);
}
else if (all.ftrl_size==6) {
msg << ":" << *v << " " << (&(*v))[1] << " " << (&(*v))[2] << " " << (&(*v))[3] << " " << (&(*v))[4] << " " << (&(*v))[5] << "\n";
brw += bin_text_write_fixed(model_file, (char*)&(*v), 6 * sizeof(*v), msg, text);
}
else if (g == nullptr || (!g->adaptive && !g->normalized))
{
msg << ":" << *v << "\n";
brw += bin_text_write_fixed(model_file, (char*)&(*v), sizeof(*v), msg, text);
Expand Down
1 change: 1 addition & 0 deletions vowpalwabbit/global_data.cc
View file
Open in desktop
Expand Up @@ -354,6 +354,7 @@ vw::vw()
normalized_updates = true;
invariant_updates = true;
normalized_idx = 2;
ftrl_size = 0;

add_constant = true;
audit = false;
Expand Down
1 change: 1 addition & 0 deletions vowpalwabbit/global_data.h
View file
Open in desktop
Expand Up @@ -565,6 +565,7 @@ struct vw
bool adaptive; // Should I use adaptive individual learning rates?
bool normalized_updates; // Should every feature be normalized
bool invariant_updates; // Should we use importance aware/safe updates
uint32_t ftrl_size;
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 pass this as an argument to save_load instead? That seems more elegant than sticking it in the global data structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I merged. Should I revert, or do you want to do a separate pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found the bug, not sure what best way to fix it, see mail

uint64_t random_seed;
uint64_t random_state; // per instance random_state
bool random_weights;
Expand Down