Skip to content

Conversation

@mmelnich
Copy link
Contributor

@mmelnich mmelnich commented Dec 2, 2025

WIP

Copy link
Contributor

@rileyjmurray rileyjmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just make a CompositeOperator class that accepts two abstract linear operators as template parameters. That would be much more flexible.

@mmelnich
Copy link
Contributor Author

mmelnich commented Dec 2, 2025

You should just make a CompositeOperator class that accepts two abstract linear operators as template parameters. That would be much more flexible.

Yeah, you're right.
It just might end up looking too complicated to read lol.

@mmelnich
Copy link
Contributor Author

mmelnich commented Dec 3, 2025

Riley says "I strongly dislike this densification. I'd ask that we log a warning to std::cerr when this codepath is hit. Later on we can figure out when's okay to be quiet about densifying."

I guess I dont need the sparse * sparse case rn, so I'l comment the densification out and add a cerr for now.

@mmelnich
Copy link
Contributor Author

For the past couple of days, I was working on figuring out an issue where CQRRT_linop with nested compisut operator input (cholsolver op, sparse op, dense op) was exhibiting poor orthogonality in the Q factor upon any sparse inputs into the cholsolver_linop (dense cases worked fine). The solution was to disable Eigen's use of fill-reduce pertmutations in sparse cholsolver.

Root Cause: Eigen's SimplicialLLT applies fill-reducing permutations (AMD/COLAMD) for truly sparse matrices. These permutations break CQRRT's numerical stability.

Solution: Force natural ordering (no permutations) by specifying Eigen::NaturalOrdering<int> as third template parameter.

@mmelnich
Copy link
Contributor Author

At the moment, there is lots of diagnostic code in the repo.
I need to do a major cleanup and then make sure that the benchmarks are well-commented and in a presentable state.

@mmelnich
Copy link
Contributor Author

This PR is completed; I will brgin breaking it up into multiple smaller PRs now.

@mmelnich mmelnich mentioned this pull request Jan 14, 2026
mmelnich added a commit that referenced this pull request Jan 19, 2026
This PR is a part of of #115, which will be broken up into more
digestible parts and closed.
This PR intoduces refined abstract linear operator support (with
testing) and adds a new "composite" linear operator type.

Core changes:

1. Linear Operators (rl_linops.hh):
- Extended DenseLinOp and SparseLinOp with full multiplication support
(both Side::Left and Side::Right)
    -   Added row-major layout support for dense operators
- Added CompositeLinOp class for operator composition (e.g., A = B * C)
    -   Added tests for each linop type

2. Matrix Generation (rl_gen.hh)
    - Added gen_random_dense() with layout support
- Added gen_sparse_coo() and gen_sparse_csc() for sparse matrix
generation
- New matrix generation functions do not rely on "mat_info" struct so
that the matrices can be generated straightforwardly

3. Utilities (rl_util.hh, rl_util_test.hh)
- Reorganized utility functions used specifically in tests into separate
header (rl_util_test.hh)
- Added sparse_to_dense_summing_duplicates() for sparse-to-dense
conversion

4. Drivers
- Minor updates to rl_abrik and its benchmarks for linop compatability
(unsure why the whole driver file appears reworked)
- rl_cqrrt is refactored to make sure it only processes full-rank
matrices
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.

3 participants