-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 issue 1093 loss value mismatch #1103
Fix issue 1093 loss value mismatch #1103
Conversation
Commit 6a10c10 resolves issue I encountered with |
… loss calculation
…d ludwig dependencies function
Given the changes for the sequence feature, this PR also fixes Issue #1096 |
This reverts commit e32ffd6
doc: update comments to reflect new naming
test: add learned_unigram to unit test
# Conflicts: # ludwig/modules/loss_modules.py # ludwig/modules/metric_modules.py
@w4nderlust This PR is finally ready for review. Following summarizes the key changes: Fix mis-match in reported loss values:
Fix for sampled_softmax_cross_entropy for sequence-like features
|
Completed rework of categorical sampled softmax tensor passing. |
Assuming the Github Actions run is clean, the PR is ready for the next round of review. |
Code Pull Requests
Fix Issue #1093
Before this PR when an model consists of single output feature, the reported loss for
combined
differs slightly from the loss reported for the feature itself. In the case of multiple output features, the sum of the individual loss for each feature differs slightly from the loss reported forcombined
. As best I can tell, this is just a reporting issue. No effect on model convergence.At this point this PR address the issue for the numeric, binary and categorical output features. Additional work is needed to propagate the fix to other output feature types.
Before PR
Here is excerpt of the Ludwig log that illustrates the issue.
b4_fix_log.txt
Here is a specific example for the numeric output feature. Note differences in the reported loss for
y1
andcombined
.For the multi output feature use case, note the sum of the individual losses for the output feature does not equal
combined
After PR
with the PR here is the same output for the single numeric feature
And with the multiple output features the sum of the losses for the multiple output features now equal the
combined
:Note: Due to rounding hand calculating the sum of individual losses may not match the displayed
combined
sum in the 4th decimal position.Here is the full log extract
after_fix_log.txt