extend typing: base #327
extend typing: base #327
Conversation
Borda
added
enhancement
Codecov Report
@@ Coverage Diff @@
## master #327 +/- ##
==========================================
- Coverage 96.59% 96.53% -0.06%
==========================================
Files 111 111
Lines 3550 3552 +2
==========================================
Hits 3429 3429
- Misses 121 123 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| return CompositionalMetric(torch.bitwise_and, self, other) | ||
|
|
||
| def __eq__(self, other: Any): | ||
| # Fixme: this shall return bool instead of Metric |
There was a problem hiding this comment.
@justusschock any idea how we could fix this?
There was a problem hiding this comment.
I discussed with @tchaton in the past. One can argue for bool and for metric (the metric checks the equality of the outputs for each argument then). Equality would probably go via is or hashes.
So the current behaviour at least is consistent with the other arithmetic and logic operators.
There was a problem hiding this comment.
yep, I see the arithmetic but feel that logic like eq shall return a bool, that is also why mypy complains as we overwrite something which shall return bool but return metric...
There was a problem hiding this comment.
How strict are we with deprecations in metrics? I'm fine with changing it. But I think checking the equality of metrics is not that easy. Probably it would require some syncing and checking the states.
There was a problem hiding this comment.
we keep 0.1 deprecation loop, but if needed we are fine with breaking changes
|
Hello @Borda! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-06-29 10:37:16 UTC |
Borda
requested review from
ananyahjha93,
justusschock,
SkafteNicki and
tchaton
as code owners
| return CompositionalMetric(torch.bitwise_and, self, other) | ||
|
|
||
| def __eq__(self, other: Any): | ||
| # Fixme: this shall return bool instead of Metric |
There was a problem hiding this comment.
I discussed with @tchaton in the past. One can argue for bool and for metric (the metric checks the equality of the outputs for each argument then). Equality would probably go via is or hashes.
So the current behaviour at least is consistent with the other arithmetic and logic operators.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
for more information, see https://pre-commit.ci
…g/metrics into refactor/typing2
for more information, see https://pre-commit.ci
…g/metrics into refactor/typing2
Before submitting
What does this PR do?
Fixes # (issue).
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃