-
Notifications
You must be signed in to change notification settings - Fork 18
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
Implement Cholesky for covariance handling #408
Implement Cholesky for covariance handling #408
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #408 +/- ##
==========================================
+ Coverage 87.90% 88.01% +0.10%
==========================================
Files 40 40
Lines 1745 1760 +15
==========================================
+ Hits 1534 1549 +15
Misses 211 211
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Interesting 😉 can you merge main (after #409). If you add |
I wanted to test with a new dataset but can't save a new one, see: pydata/xarray#8743 . |
Well now I am really confused 😅 |
macOS-latest is macOS 12 and I created the dataset on macOS 14 so let's try that. |
Well would you look at that. The dataset from macos 14 succeeds on macOS 14 and windows latest. |
Interesting that |
Yes, but they are open about it: |
It bugs me that I cannot reproduce the large difference of the datasets we get between ubuntu and macOS. The test passed locally when I test the macOS dataset on atmos (openSUSE). GitHub Actions doesn't provide openSuse but one can set up self hosted runners, see here. However, this is not recommended for public repositories for obvious reasons. Do we have an ubuntu machine where we could test where things are trailing off? Here are some observations I have made:
All of this did not get me much closer to understand why the difference between the result on ubuntu and Mac is up to 0.8. |
I tried some more stuff:
I checked ``
|
Okay but that's something. Then I'll try to pin it down with output from cfc. Thanks! |
In #168 (comment) I found that small differences (1e-15), can lead to one large difference for the "inverted" matrices (0.1), which lead to differences ind the samples. Yes, using cfc can definitely get you faster feedback. |
Okay, saving A that is used to draw from a multivariate makes the emulations pass on different OS. However, this just shifts our problem to testing the calibration where we save A. Now this will not be the same between different OS but one could argue this is less important to the user since the calibration can always be exported. A second issue here is that I was not actually able to reproduce the behavior of |
…ons" This reverts commit 5acaded.
for more information, see https://pre-commit.ci
Uff sorry the commits are a mess here, actually "fix dimensions of innovations" also includes switching from the numpy multivariate normal to the scipy multivariate normal. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
So this looks pretty good! I see two thing to do now:
|
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.
Very nice! Can you
- rename the PR
- open a issue that we re-consider the
try ... except np.linalg.LinAlgError
approach? (potentially requiremethod="eigh"
)
- test this approach with actual emulations and see if they are plausible
- also implement cholesky in the localized covariance module, but maybe in another PR? but then we would want to check again the emulations...
Yes and yes. Maybe do 1. after 2.?
Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>
for more information, see https://pre-commit.ci
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'll assume the changelog entry is for the coming PR?
edit: sorry my bad, all good
Happy to announce that the emulations look good and are approximately 10 times faster (for my small profiling script)! 😊😊 |
Hurray 😄 Did you test the training and the drawing? |
Yes, so calibrating + emulating together is 10 times faster. Drawing from the autocorrelated process is 10 times faster and finding the localized covariance is 22 times faster. |
awesome! |
Closes #402 and #168