- 
                Notifications
    You must be signed in to change notification settings 
- Fork 63
RKHS inner product #550
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?
RKHS inner product #550
Conversation
| Codecov ReportAttention:  
 
 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     
 ☔ View full report in Codecov by Sentry. | 
        
          
                examples/plot_rkhs_inner_product.py
              
                Outdated
          
        
      | ).reshape(2, 100), | ||
| np.linspace(0, 1, 100), | ||
| ).plot() | ||
|  | 
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.
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.
| plt.show() | 
        
          
                examples/plot_rkhs_inner_product.py
              
                Outdated
          
        
      | error = np.abs(computed_value - expected_value) | ||
|  | ||
| # Add new row to the dataframe | ||
| errors_df = pd.concat( | 
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 can add a new row in a Pandas dataframe just setting its value, without doing this.
        
          
                skfda/misc/rkhs_product.py
              
                Outdated
          
        
      | """ | ||
| 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: | 
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.
Doesn't this prevent that one of the FData has just one sample, for broadcasting?
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.
Right, I forgot considering broadcasting. I'll include it with the rest of changes and suggestions; working on it.
| Current test errors occur in  | 
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.
LGTM. Should it be merged?
| @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. | 
| Yes sorry, forgot to change it from draft. I think it can be merged. | 
| 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? | 
This PR adds the RKHS inner product.
Cases regarding the product between
FDataBasisobjects with different basis or with the covariance function not already expressed in the tensor basis, might be better to not consider them.