-
Notifications
You must be signed in to change notification settings - Fork 59
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
Feature/fdm #621
base: develop
Are you sure you want to change the base?
Feature/fdm #621
Conversation
Ignoring flake8 errors WPS440 and WPS441 in the doc example file |
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.
Again, sorry for taking so long reviewing this. The main functionality is great, I just have a couple of nitpicks in documentation and tests.
If you do not have the time/will to do the changes, just tell me and I will apply them myself (but please, answer my question about the comparison in the tests, as I truly do not remember which was the reference package).
>>> embedding_train = fdm.fit_transform(X=fd_train) | ||
>>> embedding_test = fdm.transform(X=fd_test) | ||
|
||
Notes: |
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 would put this note before the Parameters
section. Here is too low and it can be missed.
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 agree.
skfda/tests/test_fdm.py
Outdated
) | ||
|
||
|
||
def rbf_kernel(fd: FData, sigma: float) -> NDArrayFloat: |
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.
Cannot we use here the covariances in the covariances
module? I think this can be removed.
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.
Indeed. The fdm module was implemented before extending the covariance kernels to functional data. It makes no sense anymore, I'll change it.
skfda/tests/test_fdm.py
Outdated
def test_precalculated_grid_example( | ||
precalculated_fdatagrid_example: FDataGrid, | ||
) -> None: | ||
"""Compare the embedding in grid against the fda package.""" |
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.
Does the fda package have FDM? I do not think so. Can you put the code used to get the values in the docstring?
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.
These examples where computed by hand. I will the necessary details for reproducibility of the calculations.
skfda/tests/test_fdm.py
Outdated
) | ||
|
||
|
||
def laplacian_kernel(fd: FData, sigma: float) -> NDArrayFloat: |
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.
This is unused, remove.
Co-authored-by: Carlos Ramos Carreño <[email protected]>
Co-authored-by: Carlos Ramos Carreño <[email protected]>
I implemented and pushed all changes. |
Also, I created a web version of the study that I did on the FDM method, FDM GitHub pages |
Hey, that is cool! Yes, you can add a note on the top pointing to that page, or just reference it with a footnote cite, also at the top of the page, so that nobody misses it. |
_fdm.py
.test_fdm.py
.plot_fdm.py
.