Skip to content

Comments

Fix bug in loss for trees with one edge from leave to parent#5

Open
baumgach wants to merge 1 commit intofiveai:masterfrom
baumgach:fix_path_length_one_bug
Open

Fix bug in loss for trees with one edge from leave to parent#5
baumgach wants to merge 1 commit intofiveai:masterfrom
baumgach:fix_path_length_one_bug

Conversation

@baumgach
Copy link

@baumgach baumgach commented Dec 4, 2020

Hi there, thanks for making the code for this project public!

I think there may be a bug in the calculation of the hierarchical cross-entropy loss for the edge case of having only one edge between leaf and parent. Specifically, on this line https://github.com/fiveai/making-better-mistakes/blob/master/better_mistakes/model/losses.py#L77, j+1 is used to fill in the last column. However, when len(edges_from_leaf[i]) is 1, the loop for the denominator is never entered, so j=0, so onehot_den_np[i, :, 1] = 1.0. However, since there is only one leave, actually it should be onehot_den_np[i, :, 0] = 1.0.

The bug can be fixed by changing line 77 to this:

self.onehot_den[i, :, len(edges_from_leaf[i]) - 1] = 1.0

This should work in every case. Let me know if you agree with me.

@rhomu
Copy link

rhomu commented Sep 17, 2021

Thanks and good catch! Yes your bug fix makes sense... but it seems you have len(edges_from_leaf) instead of len(edges_from_leaf[i]) in the commit.

Happy to merge when it is fixed :-)

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.

3 participants