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

gd_mf_weights.cc has bug in writing out features for the right namespace #1871

Closed
szipkin opened this issue May 10, 2019 · 4 comments
Closed
Assignees

Comments

@szipkin
Copy link
Contributor

szipkin commented May 10, 2019

Bug:

// write out features for right namespace

The current code for the right namespace is incorrectly pointing to the left.indices, not right. As a result, it generates wrong output for the right namespace.

// write out features for left namespace
features& left = ec->feature_space[left_ns];
for (size_t i = 0; i < left.size(); ++i)
{ left_linear << left.space_names[i].get()->second << '\t' << weights[left.indicies[i]];

  left_quadratic << left.space_names[i].get()->second;
  for (size_t k = 1; k <= rank; k++)
    left_quadratic << '\t' << weights[(left.indicies[i] + k)];
}
left_linear << endl;
left_quadratic << endl;

// write out features for right namespace
features& right = ec->feature_space[right_ns];
for (size_t i = 0; i < right.size(); ++i)
{ right_linear << right.space_names[i].get()->second << '\t' << weights[left.indicies[i]];

  right_quadratic << right.space_names[i].get()->second;
  for (size_t k = 1; k <= rank; k++)
    right_quadratic << '\t' << weights[(left.indicies[i] + k + rank)];
}
right_linear << endl;
right_quadratic << endl;
@ataymano
Copy link
Member

Yes, that seems correct. Do you want to send pull request?

szipkin added a commit to szipkin/vowpal_wabbit that referenced this issue May 13, 2019
Use right indices to write out features for the right namespace, correcting bug found in VowpalWabbit#1871
@szipkin
Copy link
Contributor Author

szipkin commented May 13, 2019

It's my first time creating a pull request. Let me know if I didn't do it correctly. Thanks.

@ataymano
Copy link
Member

Thanks for creating pull request. It looks good.
But I have added one comment there - sorry, haven't noticed one mistake in initial proposal.

jackgerrits pushed a commit that referenced this issue May 14, 2019
* Update gd_mf_weights.cc

Use right indices to write out features for the right namespace, correcting bug found in #1871

* Update gd_mf_weights.cc

Add back rank to the right quadratic calculation
@jackgerrits
Copy link
Member

Looks like #1874 has resolved this. Please reopen if there is still an issue.

peterychang pushed a commit to peterychang/vowpal_wabbit that referenced this issue May 24, 2019
* Update gd_mf_weights.cc

Use right indices to write out features for the right namespace, correcting bug found in VowpalWabbit#1871

* Update gd_mf_weights.cc

Add back rank to the right quadratic calculation
JohnLangford pushed a commit that referenced this issue Jun 3, 2019
* pre-merge and remove shared features before sending examples down the reduction stack

* copy-paste error in vcxproj file

* fixing bad iterator dereference

* Enable Version and Tag override for NuGet pack (#1870)

* First attempt at softmax learner for cbadf (#1839)

* First attempt at softmax learner for cbadf

* some bugfixes and proper handling of shared and non-shared examples

* add test for cb_adf softmax

* Removing redundant safe_probability definition

* Update gd_mf_weights.cc right namespace (#1874)

* Update gd_mf_weights.cc

Use right indices to write out features for the right namespace, correcting bug found in #1871

* Update gd_mf_weights.cc

Add back rank to the right quadratic calculation

* Fixed warning message ips -> mtr (#1875)

* Fix dsjson parser regression and add smoke-test (#1878)

* Fix dsjson parser regression and add smoke-test

Note: The test is ignored under the C# Unit Tests, because the test
driver is not set up to run the native parser. This is an area we should
improve outside of the bugfix.

* Suppress test for softmax learner

* fixing bugs, finding/fixing wrong tests

* fixing finish_example, unit tests

* cleaning up CB reductions

* copying predictions up and down the stack

* removing shared example in place. Disabling finish_example

* modify unit tests to match outputs. Add patch file to undo this change later

* moving patch file to test directory

* Removing unused variables, function
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

No branches or pull requests

3 participants