Repository navigation
MAINT: Simplify beads implementation and add validation tests - #95
Merged
Merged
Conversation
Was really unnecessary to have 2 implementations when they really only diverge for two calculations. Track internal calculations so that the final baseline calculation is just a subtraction rather than requiring an additional solve. Also removed symmetric_output from _banded_dot_banded since I don't trust it.
Makes things clearer since x is typically used to denote independent variable.
Seems there's some dependance on platform or dependencies? Can't reproduce locally.
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
This PR simplifies the internal beads calculation by merging the separate banded and sparse implementations since they really only require special handling for two simple calculations. The timings before and after this change are unaffected, as shown below, with banded and sparse representing the calculation time with and without Numba, respectively.
Also provides an actual calculation of the initial cost for the first iteration so that the initial value in

params['tol_history']isn't a ridiculously large value, as shown below.Finally, adds validation tests against the MATLAB version written by the BEADS authors which this implementation was based off of.
Some notes since I haven't looked at the internals of beads in a few years:
smooth_half_windowand remove in a later follow up.Type of Pull Request
Pull Request Checklist
if applicable.