Skip to content

MAINT: Simplify beads implementation and add validation tests - #95

Merged
derb12 merged 6 commits into
developmentfrom
beads_cleanup
Sep 13, 2026
Merged

derb12 merged 6 commits into
developmentfrom
beads_cleanup

Conversation

@derb12

@derb12 derb12 commented Sep 13, 2026

Copy link
Copy Markdown
Owner

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.

Screenshot 2026-09-08 104548

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.
Screenshot 2026-09-08 104142

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:

  • Basing the exit criteria on the change in the cost function of the minimization seems kinda ... not smart? I assume I originally did it because the MATLAB version displays a figure of the cost vs iterations. A better exit criteria IMO would be to look at the change in the calculated signal. Can't really change the exit criteria now without a hard breaking change, so it's not worth it, and the change in signal vs the change in cost mirror each other to a large extent, so it's not too terrible as it is now. Maybe in the bump from v1 to v2, could revisit this though.
  • The default tol of 1e-2 seems too high. Again, not sure why I picked it, and hard to change without breaking things, so just a nit. For fits with proper parameter selection, using tol=1e-2 or tol=1e-3 only diverge by 2 or 3 iterations anyway, so it really only comes into play with improper parameters.
  • The addition of smoothing for the derivatives actually has a pretty bad effect on how convergence occurs. It's really just a band aid since if the cutoff frequency and regularization parameters are correct, the signal should have low noise and smoothing its derivatives is unnecessary. Probably want to deprecate smooth_half_window and remove in a later follow up.

Type of Pull Request

  • Bug Fix
  • New Feature
  • Miscellaneous Changes (refactor, code improvements, etc.)
  • Documentation or Example Programs

Pull Request Checklist

  • New code and/or documentation is valid for use with the BSD 3-clause license.
  • New code is fully documented with docstrings that follow Numpy style,
    if applicable.
  • New code follows PEP 8 standards as closely as possible, if applicable.
  • Added/updated tests and ensured they pass locally, if applicable.
  • [N/A] Verified that documentation builds locally, if applicable.

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.
@derb12
derb12 merged commit 9a57d94 into development Sep 13, 2026
11 checks passed
@derb12
derb12 deleted the beads_cleanup branch September 13, 2026 16:31
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.

1 participant