fix: fix #4669 by handling empty decision scores elements #4670
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ataymano
reviewed
Nov 27, 2023
test/train-sets/issue4669.txt
Outdated
| @@ -0,0 +1,50 @@ | |||
| ccb shared |User userID='aUser' | |||
There was a problem hiding this comment.
shouldn't we use simpler example here?
There was a problem hiding this comment.
I opted to add the test as is from the issue, I can add the simplified example with a -p too
There was a problem hiding this comment.
I'm going back on that. Updated the test to use your example.
ataymano
reviewed
Nov 27, 2023
| @@ -26,7 +26,8 @@ void print_update(VW::workspace& all, const VW::multi_ex& slots, const VW::decis | |||
| std::string delim; | |||
| for (const auto& slot : decision_scores) | |||
| { | |||
| pred_ss << delim << slot[0].action; | |||
| if (slot.empty()) { pred_ss << delim << "None"; } | |||
There was a problem hiding this comment.
Added it, it shows how suboptimal the empty line is. there's no way to distinguish between it and separators really... (apart from the fact you know two in sequence must be an empty action probs)
ataymano
reviewed
Nov 27, 2023
| @@ -547,6 +552,10 @@ void update_stats_ccb(const VW::workspace& /* all */, shared_data& sd, const ccb | |||
| num_labeled++; | |||
| if (i == 0 || data.all_slots_loss_report) | |||
| { | |||
| // It is possible for the prediction to be empty if there were no actions available at the time of taking the | |||
| // slot decision. In this case it does not contribute to loss. | |||
| if (preds[i].empty()) { continue; } | |||
There was a problem hiding this comment.
should we also exclude this example from denominator? (num_labeled--)
|
@jackgerrits , looks like new warning should be added to .stdout files |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4669