Skip to content

MAINT: Merge some 1D and 2D methods into ND mixin methods#73

Merged
derb12 merged 35 commits intodevelopmentfrom
merge_1d2d
Apr 4, 2026
Merged

MAINT: Merge some 1D and 2D methods into ND mixin methods#73
derb12 merged 35 commits intodevelopmentfrom
merge_1d2d

Conversation

@derb12
Copy link
Copy Markdown
Owner

@derb12 derb12 commented Apr 4, 2026

Description

Internally refactored the _Algorithm and _Algorithm2D _register (now _handle_io) wrappers and _setup_* methods to have the same signatures so that the simpler 1D and 2D methods could be generalized into ND mixin classes. This allows having just one canonical method for an algorithm rather than 2. The ND mixins are simply inherited to provide the methods to the appropriate _Algorithm[2D] subclasses.

Also refactored the PenalizedSystem, PSpline, WhittakerSystem2D and PSpline2D classes to have the same method calls so that Whittaker smoothing and P-spline smoothing could be generalized under a common penalized least squares (pls) setup. Combined with the above, this allows one method to handle 1D and 2D Whittaker smoothing and P-spline implementations, ie. one method instead of four methods.

With the above changes, this PR condenses 48 separate methods into just 17. This will be a huge help for future maintenance or changes, such as adding masking in #72, since only one code path needs updated rather than 2 or 4 for a method. Note that most of the smoothing and morphological algorithms are also low hanging fruit for this ND generalization, but the use of padded convolution in 1D makes it a bit harder to generalize (and seems quite excessive upon review). In the future, I need to see how bad edge effects are if I replace the padded convolution with just a convolution; or add a 2D padded convolution.

Tests should pick up if I screwed up majorly somewhere (eg. 2D input gets called to 1D method or a Whittaker method uses p-splines). I also generated local integration test data using this gist based on the development branch and ran the comparison tests in this gist with this PR's changes as a sanity check to ensure the 1D and 2D methods retained their output, at least against changing their main input parameters.

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.
  • Verified that documentation builds locally, if applicable.
    • viewcode links no longer get generated for the 2D methods that directly inherit from the ND mixins. Will see if it's fixable in a follow up, but it's not a big issue if not.

derb12 added 30 commits March 23, 2026 21:38
Initial look at dimension-agnostic implementations.
This way, the 2D classes don't need to have the method at all, keeping the number of docstrings I'll need to maintain the same.
1d methods converted to nd are no longer wrapped since they just call their super method internally.
All 1D and 2D penalized objects now use solve and direct_solve in a consistent way.
Easier to inherit methods now that 1d and 2d penalized objects have same attributes and call signatures.
Now handles setting up lhs and rhs internally, matching how PSpline and the 2D implementations do it.
Will allow a single call to setup Whittaker or PSpline setups for 1D and 2D.
Now one method is used for both asls and pspline_asls in both 1D and 2D.
Otherwise, the P-spline methods implemented as PLS mixins would do Whittaker smoothing.
@derb12
Copy link
Copy Markdown
Owner Author

derb12 commented Apr 4, 2026

Not related to the changes in this PR, but I'm noticing the test tests/test_whittaker.py::TestAsPLS::test_avoid_overflow_warning is emitting (intended) warnings in the CI jobs (again not seeing it locally though...). I should filter out that warning like in the similar TestAirPLS test case. And probably run tests with -W error like I do on the nightly tests for all but the min dependency case so I catch stuff like this more easily; it looks like this warning's been getted emitted as far back as the CI logs go.

@derb12
Copy link
Copy Markdown
Owner Author

derb12 commented Apr 4, 2026

Looking at the docs build, my local integration tests should've been for all methods rather than just the ones converted to ND... In commit cd0bd07, jbcd should have converted to direct_solve instead of solve since the rhs values are not multiplied by the diagonals added to the lhs, so those diagonals shouldn't have been considered as weights. Reverting to using direct_solve fixes jbcd's output, and the updated local integration tests that look at ALL methods now pass.

CI won't detect the fix, so I'll just merge this and make the change in the dev branch.

Also unrelated to this PR, but adaptive_minmax's fit in the docs is also wrong compared to the stable docs, but that goes back to 5d0f3a7. The correct fix for #62 should have been to just mask out weights == 0 or weights < eps rather than use the full weighted residual. Or maybe change adaptive_minmax to use Lagrange multipliers like its literature implementation instead of heavy weighting. Will follow up after merging. Need to add at least basic integration tests so these sort of bugs don't occur...

@derb12 derb12 merged commit ec2927f into development Apr 4, 2026
11 checks passed
@derb12 derb12 deleted the merge_1d2d branch April 4, 2026 17:12
derb12 added a commit that referenced this pull request Apr 4, 2026
As pointed out in #73, multiplying by weights really screws up the std calc if weights >> 1. Instead just mask out values with weights ~ 0.
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