-
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
Feature/fpca_regression #466
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #466 +/- ##
===========================================
+ Coverage 85.51% 85.63% +0.11%
===========================================
Files 141 143 +2
Lines 11284 11384 +100
===========================================
+ Hits 9650 9749 +99
- Misses 1634 1635 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…da into feature/FPCA_Regression
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 this functionality is implemented in the package fda.usc
. We should have a test verifying that we obtain the same results.
I have (finally) added this test. However, while going back and forth between python and R, I stumbled upon a python packet that provides a simple way to execute R code from within python. I am sceptical regarding whether or not this could be useful for the tests. On the one hand, it would ensure that the tests are testing against the correct values while forcing us to include the R source used to obtain the test reference data. On the other hand, it would make the tests considerably slower and require modifications to the GitHub actions. I think the timing tradeoff is too significant (I measured a slowdown of around 6x). Nevertheless, I thought I might just mention it just in case it comes in handy at some point. The following snippet generates the testing data for the test I just added.
|
After the speedup of the calculation of the penalty matrices the performance is almost the same
No longer applicable after the simplification of the implementation
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.
Remember to also add the class to the API reference documentation.
"""Check that the results obtained are similar to those of fda.usc. | ||
|
||
Results obtained from fda.usc with the following R code | ||
library("fda.usc") |
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.
Were you able to compare results with refund too?
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 the issue that I opened: refunders/refund#102.
It doesn't seem like we'll be getting any solutions from their side anytime soon.
""" | ||
from ...misc import inner_product_matrix | ||
|
||
if isinstance(self.fdata, FDataBasis): |
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.
Why is this specialized for FDataBasis
?
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.
Leftover from the previous implementation. Removed now.
Add the FPCA regression estimator