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

RKHS inner product #550

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

RKHS inner product #550

wants to merge 13 commits into from

Conversation

m5signorini
Copy link
Contributor

This PR adds the RKHS inner product.

Cases regarding the product between FDataBasis objects with different basis or with the covariance function not already expressed in the tensor basis, might be better to not consider them.

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (7488932) 86.67% compared to head (5c30710) 86.14%.
Report is 1 commits behind head on develop.

❗ Current head 5c30710 differs from pull request most recent head 5655385. Consider uploading reports for the commit 5655385 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #550      +/-   ##
===========================================
- Coverage    86.67%   86.14%   -0.54%     
===========================================
  Files          153      149       -4     
  Lines        12390    11783     -607     
===========================================
- Hits         10739    10150     -589     
+ Misses        1651     1633      -18     
Files Coverage Δ
skfda/misc/covariances.py 84.54% <100.00%> (+0.14%) ⬆️
skfda/tests/test_rkhs_inner_product.py 100.00% <100.00%> (ø)
skfda/misc/rkhs_product.py 89.28% <89.28%> (ø)

... and 37 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@m5signorini m5signorini marked this pull request as draft June 29, 2023 04:14
@m5signorini m5signorini marked this pull request as ready for review July 2, 2023 21:53
@m5signorini m5signorini requested a review from vnmabus July 2, 2023 21:53
examples/plot_rkhs_inner_product.py Outdated Show resolved Hide resolved
examples/plot_rkhs_inner_product.py Outdated Show resolved Hide resolved
).reshape(2, 100),
np.linspace(0, 1, 100),
).plot()

Copy link
Member

Choose a reason for hiding this comment

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

Add plt.show() after each plot. It removes the ugly Out: <whatever> lines and allows for the code to be executed outside a Jupyter notebook.

Suggested change
plt.show()

error = np.abs(computed_value - expected_value)

# Add new row to the dataframe
errors_df = pd.concat(
Copy link
Member

Choose a reason for hiding this comment

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

You can add a new row in a Pandas dataframe just setting its value, without doing this.

"""
check_fdata_dimensions(fdata1, dim_domain=1, dim_codomain=1)
check_fdata_dimensions(fdata2, dim_domain=1, dim_codomain=1)
if fdata1.n_samples != fdata2.n_samples:
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this prevent that one of the FData has just one sample, for broadcasting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I forgot considering broadcasting. I'll include it with the rest of changes and suggestions; working on it.

skfda/misc/rkhs_product.py Outdated Show resolved Hide resolved
skfda/misc/rkhs_product.py Show resolved Hide resolved
skfda/misc/rkhs_product.py Show resolved Hide resolved
skfda/misc/rkhs_product.py Show resolved Hide resolved
@m5signorini m5signorini self-assigned this Jul 24, 2023
@m5signorini m5signorini marked this pull request as draft July 31, 2023 10:19
@m5signorini
Copy link
Contributor Author

m5signorini commented Sep 1, 2023

Current test errors occur in test_pandas_fdatabasis and test_pandas_fdatagrid .
They say that the fixture all_boolean_reductions can not found.

vnmabus
vnmabus previously approved these changes Oct 13, 2023
Copy link
Member

@vnmabus vnmabus left a comment

Choose a reason for hiding this comment

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

LGTM. Should it be merged?

@vnmabus
Copy link
Member

vnmabus commented Oct 20, 2023

@m5signorini Maybe I wasn't clear. I was asking if there is anything else that you wanted to include, as the PR is marked as a draft, or if I have your permission to merge it.

@m5signorini
Copy link
Contributor Author

Yes sorry, forgot to change it from draft. I think it can be merged.

@m5signorini m5signorini marked this pull request as ready for review November 11, 2023 10:38
@vnmabus
Copy link
Member

vnmabus commented Nov 12, 2023

Thank you for your answer! However, a test is failing (and the difference between the expected and actual result is too high). Can you take a look at 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.

2 participants