Skip to content

Fix: separate Split Bregman inner solver options - #820

Merged
mrava87 merged 7 commits into
PyLops:devfrom
AHMETHAKANBEZIR1:fix/splitbregman-inner-solver-options
Oct 18, 2026
Merged

mrava87 merged 7 commits into
PyLops:devfrom
AHMETHAKANBEZIR1:fix/splitbregman-inner-solver-options

Conversation

@AHMETHAKANBEZIR1

@AHMETHAKANBEZIR1 AHMETHAKANBEZIR1 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #815.

Outer tol/rtol/rtol1 consume names needed by inner CGLS, preventing callers from configuring its stopping criteria. Add keyword-only kwargs_solver to splitbregman and SplitBregman.solve; its entries override duplicate legacy keywords without modifying the caller's dictionary.

Following review, name the compatibility catch-all **kwargs, document its planned removal in v3.0.0, and use the merged kwargs_solver consistently. Simplify the parameter documentation and show only the SciPy LSQR options actually used in the TV example.

Keep eight focused new cases using the function API and the suite's backend-aware np/assertion helpers. The CGLS accuracy and precedence tests use one outer/one inner iteration and an independent dense normal-equation solution. The relative-tolerance test documents its distinct purpose: outer rtol/rtol1 configure cost callbacks, while inner CGLS thresholds are passed through kwargs_solver.

Validation on Windows CPU / Python 3.12.14 / NumPy 2.5.4 / SciPy 1.18.1:

  • All 19 Split Bregman cases passed, including the eight new focused cases.
  • The complete sparsity module produced 100 passes and seven SPGL1 failures. All seven failures reproduce on the unchanged pre-review head 658d234 with the same environment (spgl1.py:927, np.isreal(A).all() returns a Python bool). They are not claimed resolved; no test skips or unrelated dependency changes were added.
  • The TV example completed from examples/ with Agg, exit 0. Configured pre-commit checks and diff checks passed.
  • GPU/CuPy, the full package suite and the full documentation build were not run locally. All five refreshed checks for b2143b8 passed, including Windows, Linux, Mac, Codacy and documentation.

AI disclosure: prepared and tested autonomously with OpenAI Codex. No independent human review is claimed.

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@mrava87 mrava87 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@AHMETHAKANBEZIR1 thanks for the contribution 😄

I provided some comments, mostly on tests needed some cleanup to make sure they are more in line to the other tests in the suite.

Comment thread examples/plot_tvreg.py Outdated
Comment thread examples/plot_tvreg.py Outdated
Comment thread pylops/optimization/cls_sparsity.py Outdated
Comment thread pylops/optimization/cls_sparsity.py Outdated
Comment thread pylops/optimization/sparsity.py
Comment thread pytests/test_sparsity.py Outdated
Comment thread pytests/test_sparsity.py Outdated
Comment thread pytests/test_sparsity.py Outdated
Comment thread pytests/test_sparsity.py Outdated
Comment thread pytests/test_sparsity.py Outdated
@mrava87
mrava87 merged commit 8c1cdb4 into PyLops:dev Oct 18, 2026
24 checks passed
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.

splitbregman: tol/rtol/rtol1 of the inner cgls solver cannot be set (name clash with outer parameters)

2 participants