-
Notifications
You must be signed in to change notification settings - Fork 62
This is my pull request for the issue 647 #649
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
base: develop
Are you sure you want to change the base?
Conversation
apres il faut voir si on peut copier coller les deux autres fonction dans le fichier scoring.
I build the doc finally and add the function asked
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #649 +/- ##
===========================================
- Coverage 86.83% 86.75% -0.09%
===========================================
Files 157 157
Lines 13522 13603 +81
===========================================
+ Hits 11742 11801 +59
- Misses 1780 1802 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some tests are needed for this functionality, to check with some simple examples that we are computing the right values.
hello I'm coming back after a little while of abscence I fixed the main problems and for the tests I did this: ``` def test_root_mean_squared_error_ndarray():
print("\ntest_root_mean_squared_error_ndarray\n")
print("\ntest_root_mean_squared_error_fdata\n")
print("\ntest_root_mean_squared_error_multioutput\n")
print("\ntest_root_mean_squared_log_error\n")
|
…ed_error and root_mean_squared_log_error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good.
Please, add your test functions to skfda\tests\test_scoring.py
, so that they are tested with the rest of our tests. I think the ones you showed in the comments are already in the right format (they have a test_
prefix and use asserts) but you will need to remove the print
statements. I will do a final round of review when the tests are in place.
I have added the test fonction now I hope it's good. |
skfda/misc/scoring.py
Outdated
y_pred: np.ndarray, | ||
*, | ||
sample_weight: np.ndarray | None = None, | ||
multioutput: Literal['uniform_average'] = 'uniform_average', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overloads should replace default values with ...
to avoid giving the impression hat they are taken into account (applies to every @overload
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added only for those line.
References to issues or other PRs
Describe the proposed changes
Additional information
Checklist before requesting a review