Skip to content
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

Native torch metrics #1488

Merged
merged 139 commits into from
Jun 13, 2020
Merged

Native torch metrics #1488

merged 139 commits into from
Jun 13, 2020

Conversation

justusschock
Copy link
Member

@justusschock justusschock commented Apr 14, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Starts the implementation of metric functionals.
Fixes #1295
Fixes #1297
Fixes #1296
Fixes #1302

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 🙃

Remaining Todos:

  • Modular Interface
  • docstrings
  • Tests

Regarding the commits: Everything before b193059 should already be on metrics branch, don't know why they are listed here...

@justusschock justusschock added the feature Is an improvement or enhancement label Apr 14, 2020
@justusschock justusschock self-assigned this Apr 14, 2020
@pep8speaks
Copy link

pep8speaks commented Apr 14, 2020

Hello @justusschock! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-12 16:50:54 UTC

@mergify mergify bot requested a review from a team April 14, 2020 14:06
@Borda
Copy link
Member

Borda commented Apr 14, 2020

seems like there are some additions from already merged in metrics

@justusschock
Copy link
Member Author

@Borda yes, but I don't know why. I've based this branch on metrics

@Borda
Copy link
Member

Borda commented Apr 17, 2020

@justusschock resolved the large diff, pls check it...

@justusschock
Copy link
Member Author

justusschock commented Apr 29, 2020

There are probably some minor issues with auc calculation, since roc and pr_curve are fine, but aurora and average precision aren't.
Will investigate this further

@Borda
Copy link
Member

Borda commented May 5, 2020

@williamFalcon why did you close the metrics PR? @justusschock?

@Borda
Copy link
Member

Borda commented May 26, 2020

@cuent mind to help to finish this PR?

@justusschock
Copy link
Member Author

@cuent wanted to take this over, I think. For now we've been communicating on slack about this :)

@Borda
Copy link
Member

Borda commented May 26, 2020

cuent wanted to take this over, I think. For now we've been communicating on slack about this :)

cool, let me know if I help you with something around... :]
@cuent mind drop a comment here so I could re-assign it to you...

@Borda Borda changed the base branch from metrics to master May 26, 2020 17:01
@Borda
Copy link
Member

Borda commented May 26, 2020

@justusschock mind turn it for review or rebase it?
as it is 351 commit after master and it has also 91 own commits I think that there will become collisions and sooner to resolve it better :] (also as it is root repo branch @cuent cannot commit directly, just make a PR to this branch so resolving may help)

@cuent
Copy link
Contributor

cuent commented May 26, 2020

Yeah I am rebasing and checking some of tests. There are a lot of conflicts :/ I am solving them

@Borda
Copy link
Member

Borda commented May 26, 2020

Yeah I am rebasing and checking some of tests. There are a lot of conflicts :/ I am solving them

@cuent can you push to this branch?

@cuent
Copy link
Contributor

cuent commented May 26, 2020

This PR #1962 should fix tests and rebase

@Borda
Copy link
Member

Borda commented May 28, 2020

@justusschock so this shall be closed in favour of #1962, right?

@Borda Borda added this to the 0.8.0 milestone May 28, 2020
@Borda Borda removed the ready PRs ready to be merged label Jun 12, 2020
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #1488 into master will increase coverage by 1%.
The diff coverage is 94%.

@@           Coverage Diff           @@
##           master   #1488    +/-   ##
=======================================
+ Coverage      86%     87%    +1%     
=======================================
  Files          79      81     +2     
  Lines        4960    5218   +258     
=======================================
+ Hits         4271    4547   +276     
+ Misses        689     671    -18     

@Borda Borda added the ready PRs ready to be merged label Jun 12, 2020
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team June 13, 2020 12:24
@williamFalcon williamFalcon merged commit 3436d00 into master Jun 13, 2020
@Borda Borda deleted the native_torch_metrics branch June 13, 2020 13:26
@Borda Borda restored the native_torch_metrics branch June 13, 2020 16:21
@Borda Borda deleted the native_torch_metrics branch June 13, 2020 16:22
@Borda Borda mentioned this pull request Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement help wanted Open to be worked on ready PRs ready to be merged
Projects
None yet
6 participants