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

add roc curve #72

Merged
merged 2 commits into from
Nov 1, 2023
Merged

add roc curve #72

merged 2 commits into from
Nov 1, 2023

Conversation

nicholasloveday
Copy link
Collaborator

closes #46

@nicholasloveday nicholasloveday self-assigned this Oct 5, 2023
@nicholasloveday
Copy link
Collaborator Author

nicholasloveday commented Oct 5, 2023

This is still work in progress.
TODO

  • Finish roc_curve_data metric unit tests
  • Raise error if values outside [0,1] in ROC
  • Update dim handling in ROC
  • Update ROC docstring
  • Jupyter notebook

@nicholasloveday
Copy link
Collaborator Author

It also may need to be updated if #71 is merged in first.

@nicholasloveday nicholasloveday force-pushed the roc_curves branch 3 times, most recently from 16115d8 to e3e6d0d Compare October 6, 2023 04:27
@nicholasloveday nicholasloveday force-pushed the roc_curves branch 6 times, most recently from d17e987 to 6d09906 Compare October 18, 2023 06:20
@nicholasloveday
Copy link
Collaborator Author

Since this is a large pull request, here is the suggested order that I think would be good to tackle the code review.

  1. Start with the tutorial notebook to get an overview
  2. Review the updated processing code
  3. Review the categorical binary metrics
  4. Review the ROC implementation.

@nicholasloveday nicholasloveday marked this pull request as ready for review October 18, 2023 23:10
Copy link
Collaborator

@aidanjgriffiths aidanjgriffiths left a comment

Choose a reason for hiding this comment

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

Hey @nicholasloveday, there has been some changes to the test directory structure changes in develop which will resolve in some conflicts from #77. Theres a couple of suggetions in this review but its best if I do a more in depth review once the test suite conflicts are sorted out.

src/scores/categorical/binary_impl.py Outdated Show resolved Hide resolved
src/scores/probability/roc_impl.py Outdated Show resolved Hide resolved
src/scores/probability/roc_impl.py Show resolved Hide resolved
@nicholasloveday
Copy link
Collaborator Author

Thanks @aidanjgriffiths - I've rebased and resolved conflicts. Ready for you to have another look at it.

Copy link
Collaborator

@aidanjgriffiths aidanjgriffiths left a comment

Choose a reason for hiding this comment

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

Looks good @nicholasloveday!

src/scores/probability/roc_impl.py Show resolved Hide resolved
tests/categorical/test_multicategorical.py Show resolved Hide resolved
Copy link
Collaborator

@tennlee tennlee left a comment

Choose a reason for hiding this comment

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

I have left a small number of comments but they require addressing. Thanks for this significant contribution.

src/scores/categorical/binary_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/binary_impl.py Outdated Show resolved Hide resolved
src/scores/processing.py Outdated Show resolved Hide resolved
src/scores/processing.py Outdated Show resolved Hide resolved
src/scores/processing.py Outdated Show resolved Hide resolved
src/scores/processing.py Outdated Show resolved Hide resolved
src/scores/categorical/binary_impl.py Outdated Show resolved Hide resolved
@nicholasloveday
Copy link
Collaborator Author

Thanks @tennlee, I've made those changes.

@tennlee tennlee merged commit b94731b into develop Nov 1, 2023
4 checks passed
@tennlee tennlee deleted the roc_curves branch November 1, 2023 01:50
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.

Add ROC curve
3 participants