Add NestedVariogram model #33

juliohm · 2020-10-21T14:36:47Z

Fix JuliaEarth/GeoStats.jl#127, JuliaEarth/GeoStats.jl#126

This PR is work in progress. Everything is working except for the fact that the return type of the model is UniformScaling. I am trying to find a way to return a simple Number when the coefficients are matrices with a single entry.

codecov-io · 2020-10-21T17:13:41Z

Codecov Report

Merging #33 into master will increase coverage by 21.14%.
The diff coverage is 74.07%.

@@             Coverage Diff             @@
##           master      #33       +/-   ##
===========================================
+ Coverage   68.67%   89.81%   +21.14%     
===========================================
  Files          11       13        +2     
  Lines         399      432       +33     
===========================================
+ Hits          274      388      +114     
+ Misses        125       44       -81

Impacted Files	Coverage Δ
src/Variography.jl	`100.00% <ø> (ø)`
src/nested.jl	`70.83% <70.83%> (ø)`
src/fitting.jl	`100.00% <100.00%> (ø)`
src/theoretical.jl	`98.07% <100.00%> (+3.95%)`	⬆️
src/plotrecipes/theoretical.jl	`90.90% <0.00%> (+90.90%)`	⬆️
src/plotrecipes/hscatter.jl	`93.02% <0.00%> (+93.02%)`	⬆️
src/plotrecipes/empirical.jl	`94.73% <0.00%> (+94.73%)`	⬆️
src/plotrecipes/varioplane.jl	`96.00% <0.00%> (+96.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 343b4a7...1e16d83. Read the comment docs.

juliohm · 2020-10-21T17:20:01Z

This PR is ready for review. Appreciate if you can take a look before we go ahead and merge it. The main question I have is about the sill and nugget of a nested model with matrix coefficients. What do you think we should return? How do you plan to use the model in downstream solvers? Perhaps returning the diagonal of the matrix would make sense, or something else that fits better with use cases.

After this PR is merged, we can open a separate issue to discuss and implement the canonical form transformation.

rmcaixeta

Very nice. Only reading the code I was skeptical that all that mix of matrix and scalars would work. I cloned and tested with some univariate nested scenarios comparing against GSLIB vmodel benchmarks I had. All working nicely. Only got error with some strange structures (details above). For my workflow downstream it helps a lot this way of looping structures. For future cokriging I expect that these matrix forms will work as they are after a canonical normalization of the sills. Some constraints might be necessary later in the solvers to adapt to eventual matrix format outputs, but that's for the future too

src/nested.jl

exepulveda

Thanks for you PR. See my review. Cheers

src/Variography.jl

src/nested.jl

juliohm · 2020-10-22T00:39:03Z

For some reason GitHub isn't allowing me to comment on your last comment directly @exepulveda. I will reply here as a separate comment:

If we adopt my proposal of using NestedVariogram{N,M}, this situation can be managed transforming 10 into a diagonal matrix of size M. Also we can check consistency of cs elements.

We can still perform checks on the constructor of the NestedVariogram without the type parameter M. What are the admissible types of coefficients? I am thinking about checking for the PSD property of the matrices.

exepulveda · 2020-10-22T01:53:03Z

For some reason GitHub isn't allowing me to comment on your last comment directly @exepulveda. I will reply here as a separate comment:

If we adopt my proposal of using NestedVariogram{N,M}, this situation can be managed transforming 10 into a diagonal matrix of size M. Also we can check consistency of cs elements.

We can still perform checks on the constructor of the NestedVariogram without the type parameter M. What are the admissible types of coefficients? I am thinking about checking for the PSD property of the matrices.

Coefficients can be scalars for the univariate case or squared matrices for multivariate case.

juliohm · 2020-10-22T02:00:47Z

Coefficients can be scalars for the univariate case or squared matrices for multivariate case.

These squared matrices need to be symmetric? I have to review the co-Kriging literature, it is been a while.

juliohm · 2020-10-22T02:10:28Z

So it seems we have two issues left to be solved:

Handle the coefficient multiplication in the multivariate case.
Handle the canonical form in general.

The first issue seems to require a clear statement about what types of coefficients are permitted and if we want to allow for a mixed approach (scalar + matrix). My plan is to merge the PR by tomorrow, and then think about the multivariate case when we need it. Please let me know if that works. The second issue is on my TODO list already.

exepulveda · 2020-10-22T09:39:21Z

Coefficients can be scalars for the univariate case or squared matrices for multivariate case.

These squared matrices need to be symmetric? I have to review the co-Kriging literature, it is been a while.

For a LMC yes, they need to be symmetric and positive semi definite See here for more details

test/nested.jl

juliohm · 2020-10-22T14:44:25Z

For a LMC yes, they need to be symmetric and positive semi definite See here for more details

That is a nice link @exepulveda , thanks for sharing. I am taking care of the other suggestions you raised.

rmcaixeta · 2020-10-22T21:29:04Z

So it seems we have two issues left to be solved:
1. Handle the coefficient multiplication in the multivariate case.

2. Handle the canonical form in general.
The first issue seems to require a clear statement about what types of coefficients are permitted and if we want to allow for a mixed approach (scalar + matrix). My plan is to merge the PR by tomorrow, and then think about the multivariate case when we need it. Please let me know if that works. The second issue is on my TODO list already.

For (1), I think in practice we'll always work with same type of coefficients:

univariate: only scalars coefficients
two vars: only 2d matrices coefficients
three vars: only 3d matrices coefficients
etc...

IMO, it's better to let it really crash if the user mix the coefficients

juliohm · 2020-10-22T22:23:43Z

After adding more tests as suggested by @exepulveda, I went ahead and implemented the canonical form in this same PR to speed up things and don't block your research. The structures function returns a tuple with the canonical form:

γ = SphericalVariogram() + 2*ExponentialVariogram() + NuggetEffect(10.0)
@test structures(γ) == (10.0, (1.0, 2.0), (SphericalVariogram(), ExponentialVariogram()))

Appreciate if you can review and/or add more tests before we merge it. The current behavior makes sense?

rmcaixeta

Fine for me. It'll help a lot, thanks guys

juliohm · 2020-10-23T20:08:56Z

Thank you guys for the input. I will roll out a new release with the NestedVariogram and the new aniso2distance feature. 👍

Add NestedVariogram

3074de6

juliohm marked this pull request as draft October 21, 2020 14:39

juliohm added 2 commits October 21, 2020 13:49

Handle UniformScaling

aa6a856

Add tests for matrix coefficients

37e5e02

juliohm requested review from exepulveda and rmcaixeta October 21, 2020 17:16

juliohm marked this pull request as ready for review October 21, 2020 17:16

juliohm added 2 commits October 21, 2020 14:25

Fix IO for NestedVariogram

12c408b

More fixes for NestedVariogram IO

e3bb9e9

rmcaixeta reviewed Oct 21, 2020

View reviewed changes

src/nested.jl Outdated Show resolved Hide resolved

exepulveda reviewed Oct 22, 2020

View reviewed changes

src/Variography.jl Show resolved Hide resolved

src/nested.jl Outdated Show resolved Hide resolved

src/nested.jl Show resolved Hide resolved

src/nested.jl Outdated Show resolved Hide resolved

src/nested.jl Show resolved Hide resolved

Rename models to structures in IO methods

2f66802

Repository owner deleted a comment from exepulveda Oct 22, 2020

juliohm added 2 commits October 21, 2020 22:04

Check for symmetry of coefficients

cbb423b

Refactor tests

80716e3

exepulveda suggested changes Oct 22, 2020

View reviewed changes

test/nested.jl Show resolved Hide resolved

Add test for NestedVariogram constructor

6b410ac

juliohm added 3 commits October 22, 2020 18:31

Format code in nested.jl

7617981

Add structures function

8f702f3

Fix IO methods

24e737b

juliohm requested review from exepulveda and rmcaixeta October 22, 2020 22:24

juliohm added 2 commits October 22, 2020 19:33

Improve docstring for structures function

d92f013

Fix formatting

1e16d83

exepulveda approved these changes Oct 23, 2020

View reviewed changes

rmcaixeta approved these changes Oct 23, 2020

View reviewed changes

juliohm merged commit 0ee9676 into master Oct 23, 2020

juliohm deleted the nested-variogram branch October 23, 2020 20:07

Add NestedVariogram model #33

Add NestedVariogram model #33

Uh oh!

Conversation

juliohm commented Oct 21, 2020

Uh oh!

codecov-io commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

juliohm commented Oct 21, 2020

Uh oh!

rmcaixeta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

exepulveda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliohm commented Oct 22, 2020

Uh oh!

exepulveda commented Oct 22, 2020

Uh oh!

juliohm commented Oct 22, 2020

Uh oh!

juliohm commented Oct 22, 2020

Uh oh!

exepulveda commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

juliohm commented Oct 22, 2020

Uh oh!

rmcaixeta commented Oct 22, 2020

Uh oh!

juliohm commented Oct 22, 2020

Uh oh!

rmcaixeta left a comment

Choose a reason for hiding this comment

Uh oh!

juliohm commented Oct 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Oct 21, 2020 •

edited

Loading

exepulveda commented Oct 22, 2020 •

edited

Loading