Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Fix DeepMIL metrics bug #674

Merged
merged 7 commits into from
Mar 1, 2022
Merged

Fix DeepMIL metrics bug #674

merged 7 commits into from
Mar 1, 2022

Conversation

dccastro
Copy link
Member

PR in draft mode pending merging of microsoft/hi-ml#195 to update the submodule.

Fixes a bug in DeepMILModule whereby predicted hard labels were being passed to classification metrics, instead of the intended continuous probability scores. This PR also adds tests to ensure this won't happen anymore.

@dccastro dccastro changed the title [WIP] Fix DeepMIL metrics bug Fix DeepMIL metrics bug Feb 28, 2022
@dccastro dccastro marked this pull request as ready for review February 28, 2022 15:23
maxilse
maxilse previously approved these changes Feb 28, 2022
@vale-salvatelli
Copy link
Contributor

For my own understanding - why the hi-ml update is needed for this PR?
I saw the PR in hi-ml to add the same change in there but I guess the two things go in parallel. Am I missing other changes?

@dccastro
Copy link
Member Author

dccastro commented Feb 28, 2022

No other crucial changes in hi-ml related to this PR, but thought it prudent to keep it up-to-date (in particular both containing the same fixes). I can revert it if you prefer.

for key in thresholded_metrics_keys}

for key in thresholded_metrics_keys:
assert not torch.allclose(results_low_threshold[key], results_high_threshold[key]), \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default tolerance is quite low, can make sense to pass it a larger number given here we're interested in checking these results ARE different?

@vale-salvatelli
Copy link
Contributor

No need to revert - I was just trying to understand.

@maxilse maxilse merged commit e984554 into main Mar 1, 2022
@maxilse maxilse deleted the dacoelh/fix_metrics branch March 1, 2022 09:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants