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

fix: update to reflect the fact that coin's learn does not return a prediction #4621

Merged
merged 3 commits into from
Aug 31, 2023

Conversation

jackgerrits
Copy link
Member

@jackgerrits jackgerrits commented Jul 7, 2023

The prediction it currently returns essentially represents what the prediction would have been post learn and therefore does not match the contract of learn_returns_prediction.

The prediction it currently returns essentially represents what the prediction would have been post learn and therefore does not match the contract of learn_returns_prediction,
@ataymano
Copy link
Member

Essence of the issue:
data.txt:

1 | x:1
1 | x:1
1 | x:1
-100 | x:10
vw -d data.txt -p pred.txt --quiet --coin --noconstant

pred.txt:

0
1.000000
1
0.181818

The fact that last prediction got decreased indicates that it is not pre-learn predict but the one affected by learning on -100.
Although looks like this change is not enough and final prediction file still get overwritten prediction from learn stack

@ataymano
Copy link
Member

Just checked manually - example from above got passed.

@ataymano ataymano merged commit bea6816 into master Aug 31, 2023
115 checks passed
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.

2 participants