-
Notifications
You must be signed in to change notification settings - Fork 3
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
add pairwise! for Covariance types #5
Conversation
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.
The test with CartesianGrid allocates because it computes covariance between the elements of the domain, in this case quadrangles. In order to compute the covariance between two quadrangles, the variogram samples points inside the quadrangles based on the range, and performs numerical integration.
The variogram and covariance don't require allocation when they are evaluated with points. For example, if you construct PointSet(centroid.(grid)) as your domain, that will be much faster and shouldn't allocate.
Please add tests here as well for coverage purposes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
==========================================
- Coverage 85.03% 84.36% -0.68%
==========================================
Files 26 26
Lines 628 633 +5
==========================================
Hits 534 534
- Misses 94 99 +5 ☔ View full report in Codecov by Sentry. |
src/covariance.jl
Outdated
pairwise(cov, domain₁, domain₂) | ||
|
||
Evaluate covariance `cov` between all elements of `domain₁` and `domain₂`. | ||
""" | ||
pairwise(cov::Covariance, args...) = sill(cov.γ) .- pairwise(cov.γ, args...) | ||
|
||
""" | ||
pairwise!(Γ, cov, domain) |
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.
We denote the covariance matrix by C or \Sigma. \Gamma is reserved for the variogram matrix.
Left a small comment regarding notation. Can you please replace \Gamma --> C or \Sigma in the covariance methods? Also, it would be nice to investigate this allocation with Point in another PR. We probably need an extra method to avoid sampling the trivial geometry (i.e., point). |
@markmbaum this PR has conflicts with the other PR that got merged. |
This adds the
pairwise!
function forCovariance
types. I also added a docstring for the variogram'spairwise!
method.closes JuliaEarth/GeoStats.jl#405
I did also notice that the existing implementation allocates a lot of memory even though it should just be filling a destination matrix. For example:
I'm not exactly sure why and I didn't want to mess with the original implementation.