Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Class based solvers #90

Closed
mrava87 opened this issue Jun 16, 2019 · 1 comment
Closed

Class based solvers #90

mrava87 opened this issue Jun 16, 2019 · 1 comment
Labels
breaking Breaking change Core Pulls for core PyLops library

Comments

@mrava87
Copy link
Collaborator

mrava87 commented Jun 16, 2019 •
edited
Loading

Change

Move from function based to class based solvers. This would require creating a base class with the following:

  • __init__
  • step: run a single step
  • callback: run after any step (a user can just overwrite it - currently is passed to the solver as function)
  • print: print update
  • solve: run entire solver

Motivation

  • This allows running multiple inversion with different solver parametrization (niter, tol, etc) without having to rerun the preparation part, which could be time consuming and independent on the solver parameters.
  • This allows users to run both the solver as is or run it in a more fine-grained fashion (using step).

Countermotivation

  • This is a breaking change, is it worth?
  • Scipy has function based solvers, if we move away from them (for also cg, cgls, lsqr) we will need to call them differently inside our apps depending on whether we use numpy or cupy arrays - right now we just call a different solver but with a very similar function signature.
@mrava87 mrava87 added the Core Pulls for core PyLops library label Dec 29, 2021
@mrava87 mrava87 added the breaking Breaking change label Feb 5, 2022
@marcusvaltonen marcusvaltonen mentioned this issue Mar 23, 2022
Closed
@marcusvaltonen
Copy link

This is really the way to go. I just want to add code reusability as part of the motivation.

mrava87 added a commit to mrava87/pylops that referenced this issue Apr 9, 2022
@mrava87
feature: created class-based template Solver and CG
2a61692 
Based on PyLops#90, this is the first attempt
at creating class-based solvers. In this commit we include both a template
class which every solver should subclass from and a first implementation of CG.
@mrava87 mrava87 mentioned this issue Apr 9, 2022
Merged
cako added a commit that referenced this issue May 13, 2022
@mrava87 @cako
Class-based solvers (#363)
5dc077e 
* feature: created class-based template Solver and CG
Based on #90, this is the first attempt
at creating class-based solvers. In this commit we include both a template
class which every solver should subclass from and a first implementation of CG.

* minor: continue creating class-based solvers
This commit introduces some modifications to the Solver and CG classes
based on this discussion #363.

* minor: return values from solve method

* minor: modify handling of y and x in Solver

* minor: more flexibility in choosing steps to print

* Added CGLS class

* fix: raw strings

* feature: added class-based leastsquares solvers

* feature: converted ISTA to class-based solver

* feature: coverted all sparse solvers to class-based and added Callbacks class

* minor: small flake8 changes

* minor: fix iteration print in ISTA

* feature: Decorator-based calls to callbacks

* Update basesolver.py

chore: remove unused import

* Update basesolver.py

Co-authored-by: Carlos da Costa <[email protected]>
mrava87 added a commit to mrava87/pylops that referenced this issue May 15, 2022
@mrava87
minor: finalize new optimization restyling
f51a004 
This commit finalizes some of the remaining tasks in PyLops#363
and closes PyLops#90.
@cako cako closed this as completed in 080b911 May 16, 2022
@cako cako mentioned this issue May 16, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking Breaking change Core Pulls for core PyLops library
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@marcusvaltonen @mrava87