Skip to content

Take 2: Vecchia.jl extension to offload sparsity pattern design.#133

Merged
amontoison merged 6 commits intomasterfrom
cg/vecchiajl_ext
Jan 16, 2026
Merged

Take 2: Vecchia.jl extension to offload sparsity pattern design.#133
amontoison merged 6 commits intomasterfrom
cg/vecchiajl_ext

Conversation

@cgeoga
Copy link
Copy Markdown
Collaborator

@cgeoga cgeoga commented Jan 15, 2026

After botching a rebase in my first attempt at this PR, here is a clean one including a passing test.

In an incoming commit, I'll remove some unnecessary code in this tree.

To be very clear about what this extension does, it only offloads the work of obtaining the sparsity pattern (I,J) from a collection of locations and conditioning set design. In Vecchia.jl, I have already implemented a variety of different options for doing this and expect to have several more useful ones coming into that codebase shortly. And since that functionality will be tested and expanded there, I think it makes more sense to depend on it in this code rather than hard-coding one option here with some slightly difficult logic to follow.

@cgeoga
Copy link
Copy Markdown
Collaborator Author

cgeoga commented Jan 15, 2026

Okay, @amontoison, here is the big update. If we agree on offloading the specific Vecchia-style generation of a sparsity pattern to the Vecchia.jl extension, then we can remove a significant amount of code here. I also removed the hard-coded sampling functions, which were a huge number of lines of code and aren't really functionality that needs to be in the core of this package. You can see in runtests.jl I wrote a perfectly fine code for testing that generates samples that is a few lines of code.

Additionally, you'll see that in ./src/models/VecchiaModel.jl I added a couple constructors where you just hand in an UpperTriangular(sparse_U) or LowerTriangular(sparse_L) as another option for communicating the sparsity pattern. Between that and handing in the (I,J) coo format, I think that is pretty flexible. Although now that I write this, maybe the model could be built from format=:csc if one is already handing in a sparse matrix object?

Many of the tests use old functions for generating samples, but I updated one (test_model_solver.jl) to demonstrate how I would go through and update the rest. Hopefully this is agreeable: fewer lines of code, simpler logic, etc.

What are your thoughts? If this is all agreeable, I'll go through and update the rest of the tests and add some docstrings and stuff (and update the new VecchiaModel constructors to use format=:csc if my suggestion above is correct).

@amontoison
Copy link
Copy Markdown
Member

Okay, @amontoison, here is the big update. If we agree on offloading the specific Vecchia-style generation of a sparsity pattern to the Vecchia.jl extension, then we can remove a significant amount of code here. I also removed the hard-coded sampling functions, which were a huge number of lines of code and aren't really functionality that needs to be in the core of this package. You can see in runtests.jl I wrote a perfectly fine code for testing that generates samples that is a few lines of code.

Additionally, you'll see that in ./src/models/VecchiaModel.jl I added a couple constructors where you just hand in an UpperTriangular(sparse_U) or LowerTriangular(sparse_L) as another option for communicating the sparsity pattern. Between that and handing in the (I,J) coo format, I think that is pretty flexible. Although now that I write this, maybe the model could be built from format=:csc if one is already handing in a sparse matrix object?

Many of the tests use old functions for generating samples, but I updated one (test_model_solver.jl) to demonstrate how I would go through and update the rest. Hopefully this is agreeable: fewer lines of code, simpler logic, etc.

What are your thoughts? If this is all agreeable, I'll go through and update the rest of the tests and add some docstrings and stuff (and update the new VecchiaModel constructors to use format=:csc if my suggestion above is correct).

@cgeoga I like these modifications, you have my green light to continue!
For the wrappers UpperTriangular(sparse_U) or LowerTriangular(sparse_L), the main issue is that the full matrix can be stored and nothing is checked to ensure that we have only provide one triangle.
I prefer the user to be aware of this and don't help him too much.

@amontoison
Copy link
Copy Markdown
Member

Although now that I write this, maybe the model could be built from format=:csc if one is already handing in a sparse matrix object?

Yes, you can use directly format=:csc and provide A.colptr and A.rowval where A is the sparse matrix in the triangular wrapper.

@amontoison
Copy link
Copy Markdown
Member

@cgeoga Once you fixed the test, I merge your modifications.

@amontoison amontoison merged commit c122771 into master Jan 16, 2026
3 of 7 checks passed
@amontoison amontoison deleted the cg/vecchiajl_ext branch January 16, 2026 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants