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.

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

Refactor the design of the probabilistic linear solver functions and classes #51

Open
1 of 2 tasks
JonathanWenger opened this issue Apr 21, 2020 · 2 comments
Open
1 of 2 tasks
Assignees
Labels
linalg Issues related to linear algebra refactoring Refactoring of existing functionality

Comments

@JonathanWenger
Copy link
Contributor

JonathanWenger commented Apr 21, 2020

  • Cleanly separate _preprocess_linear_system from _init_solver and make sure only one takes the assumptions on A, i.e. assume_A as an argument.

  • Create a common (abstract) class MatrixBasedSolver(ProbabilisticLinearSolver) with generic functions:

    • policy
    • observe
    • step_size / line_search
    • infer_posteriors
    • optim_hyperparams
    • solve
    • ...
      This way we can avoid some code duplication between the different versions of the matrix-based solvers.
@JonathanWenger JonathanWenger added refactoring Refactoring of existing functionality linalg Issues related to linear algebra labels Apr 21, 2020
@JonathanWenger JonathanWenger self-assigned this Apr 21, 2020
@JonathanWenger JonathanWenger added this to the Initial Release milestone Jul 3, 2020
@nathanaelbosch
Copy link
Collaborator

nathanaelbosch commented Aug 18, 2020

In the initial pylint PR #150 I found many pylint messages that mostly relate to linalg, and especially matrixbased. I'll post a full list in here since it seems to relate to this PR. If you think that those are better tackled through individual tasks I could also open individual issues:

  • too-many-arguments
  • abstract-method
  • too-many-branches
  • arguments-differ
  • redefined-builtin
  • too-many-lines
  • abstract-class-instantiated
  • too-complex
  • too-many-instance-attributes
  • too-many-statements
  • attribute-defined-outside-init

These are not all the errors that pylint raises, neither are those errors that only appear in linalg, but I think that most of these should be considered when working on this refactoring, and ideally I would hope for pylint src/probnum/linalg/ to pass with these messages enabled (e.g. through the --enable=<message> argument).

@nathanaelbosch
Copy link
Collaborator

Relating to my previous comment: I just opened #158 to track the work on the pylint exceptions for the linalg module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linalg Issues related to linear algebra refactoring Refactoring of existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants