-
Notifications
You must be signed in to change notification settings - Fork 58
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
Covariance kernels #601
base: develop
Are you sure you want to change the base?
Covariance kernels #601
Conversation
Converted tests from old test_covariances file to pytest format and added tests for functional data objects.
Adapted Linear, Polynomial, Exponential, Gaussian and Matern covariances and added control messages for covariance kernels that do not support functional data objects.
Changed function to test multivariate data for all covariance functions. Now uses ParameterGrid.
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
pep8
skfda/misc/covariances.py|822 col 1| D102 Missing docstring in public method
skfda/misc/covariances.py|865 col 1| D102 Missing docstring in public method
skfda/misc/covariances.py|872 col 1| W293 blank line contains whitespace
skfda/misc/covariances.py|873 col 9| WPS428 Found statement that has no effect
skfda/misc/covariances.py|873 col 9| WPS462 Wrong multiline string usage
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.
You have changed spacing in all files. Please, make sure that your IDE is using Unix end lines (LF) instead of Windows ones (CRLF), so that only the lines with ACTUAL changes show in the diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #601 +/- ##
===========================================
- Coverage 86.65% 86.65% -0.01%
===========================================
Files 156 156
Lines 13314 13356 +42
===========================================
+ Hits 11537 11573 +36
- Misses 1777 1783 +6 ☔ View full report in Codecov by Sentry. |
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.
Some changes. Take a look also to the style.
skfda/tests/test_covariances.py
Outdated
fd = fetch_functional_data | ||
cov_kernel = covariances_raise_fixture | ||
|
||
np.testing.assert_raises( |
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.
It is better to use pytest.raises
as a context manager: https://docs.pytest.org/en/7.1.x/how-to/assert.html#assertions-about-expected-exceptions
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.
It was meant to be used as a context manager (in a with
statement).
What is the status of this (and subsequent) PRs? Do you have time to take a look at my comments now, @E105D104U125, so that we can move forward and merge this? |
Changed covariances.py from misc module in order to generalize the Linear, Polynomial, Exponential, Gaussian and Matern kernels for FData.
Included tests for theses changes and adapted the already existing tests for multivariate data to pytest format.