Merged
Conversation
Used the modifications in pentapy issue 11 and PR 27 to improve indexing and error handling of the solvers. These versions are ~20-35% faster than pentapy's current versions.
Much easier to use it as a drop-in replacement for solve_banded and does not incur a significant time penalty.
Makes it easier to follow along when cross-checking with the reference paper.
Also added preliminary tests for the pentadiagonal solvers.
Since using_pentapy is no longer a valid way to tell if matrices are row-wise banded, need to cause a breaking change to any code that uses it. Likely not used by outside users, but this ensures the new behavior is tracked.
Uses 2N less additions within the inner loop (time saving negligible), but more importantly unifies the code for the direct solvers and factorizations.
Owner
Author
|
The failing doctest was introduced in the development branch, not this one, so I'll fix it after merging this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Vendors the PTRANS-I and PTRANS-II solvers from pentapy as private functions. The solvers were internally changed following the guidelines presented in pentapy issue 11 and pull request 27, as well as to fit some personal preferences. Changes outlined as follows:
Points (2) and (3) aren't of use in pybaselines yet, but the
optimize_lambranch will benefit from them.The
using_pentapyattribute of _banded_utils.PenalizedSystem was renamed tousing_penta. It didn't need to be done, but a lot of my personal code uses the PenalizedSystem object and would otherwise potentially be using banded matrices in an unexpected band arrangement without any warning (why point (1) was done), so I thought it'd be better to cause any code using that attribute to fail rather than silently be in an unexpected band format.Concerning performance, the direct solver is ~20-30% faster than the current main branch of pentapy thanks to improved indexing and solving without tracking the full factorization, both explained in pentapy issue 11. Both the direct solver and factorized solver produce identical outputs to pentapy's solvers, at least on all of the different Whittaker smoothing systems I've tested. Timings and correctness can be compared with the following gist: https://gist.github.com/derb12/1eff076884a5512fb5e833e47cea91bc
Type of Pull Request
Pull Request Checklist
if applicable.