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

Issue #28, StandardizedErrorReason class #29

Merged
merged 6 commits into from
Dec 21, 2021
Merged

Issue #28, StandardizedErrorReason class #29

merged 6 commits into from
Dec 21, 2021

Conversation

FBruzzesi
Copy link
Contributor

Implementation of StandardizedErrorReason class for regression tasks, as disccussed in issue #28
Other file changes are due to black formatting

Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

There's a few minor items to tick off.

  • The readme.md file and as well the quickstart guide in the docs list all the reasons in the package. Feel free to add this new one.
  • If you have a look at some of the classifier reasons like this one you'll notice that a from_predict method is implemented. This is nice to have because the end user can calculate the predictions once and re-use them across reasons. It also makes it much easier to write unit tests. Could you add such a method as well as a unit test?

Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

There's really just one nitpick. But other than that, looks great!

@koaning koaning merged commit 9202b8a into koaning:main Dec 21, 2021
@koaning
Copy link
Owner

koaning commented Dec 21, 2021

Grand! Let's merge it!

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.

None yet

2 participants