Skip to content

Solver mixin#804

Open
GiovanniCanali wants to merge 3 commits into
mathLab:0.3from
GiovanniCanali:solver_mixin
Open

Solver mixin#804
GiovanniCanali wants to merge 3 commits into
mathLab:0.3from
GiovanniCanali:solver_mixin

Conversation

@GiovanniCanali
Copy link
Copy Markdown
Collaborator

@GiovanniCanali GiovanniCanali commented May 29, 2026

Description

This PR fixes #777

TODO list:

  • Fix docstring for all solver-related files
  • Add one-line doc header
  • Add rst files
  • Add old PINN variants + tests

Checklist

  • Code follows the project’s Code Style Guidelines
  • Tests have been added or updated
  • Documentation has been updated if necessary
  • Pull request is linked to an open issue

@GiovanniCanali GiovanniCanali self-assigned this May 29, 2026
@GiovanniCanali GiovanniCanali added enhancement New feature or request pr-to-fix Label for PR that needs modification 0.3 Related to 0.3 release labels May 29, 2026
@GiovanniCanali GiovanniCanali force-pushed the solver_mixin branch 3 times, most recently from ed9f244 to f375911 Compare May 29, 2026 14:33
@GiovanniCanali
Copy link
Copy Markdown
Collaborator Author

BaseSolver, SingleModelSolver, MultiModelSolver, and EnsembleSolver are base classes that aggregate one or more mixins into a single behavioral unit. They are intended to be specialized by concrete solver implementations that inherit from them.

@dario-coscia
Copy link
Copy Markdown
Collaborator

@GiovanniCanali I write here as I see stuff but I did not finish full review:

  1. I would divide mixing and solvers in the doc
Screenshot 2026-06-04 at 17 42 26
  1. Why are the mixing classes with _? They are not private; people will use them.
  2. Why do we remove the reduction in the loss here?
  3. The _EnsembleMixin forward shuould call the forward super of _MultiModelMixin and mean the super forward result.
  4. How do you envision the integration of weighting, both per residual sample and per conditional components?
  5. I would remove typechecker to a safer approach, like pydantic models.

@GiovanniCanali
Copy link
Copy Markdown
Collaborator Author

@GiovanniCanali I write here as I see stuff but I did not finish full review:

  1. I would divide mixing and solvers in the doc
Screenshot 2026-06-04 at 17 42 26 2. Why are the mixing classes with `_`? They are not private; people will use them. 3. Why do we remove the reduction in the loss [here](https://github.com/GiovanniCanali/PINA/blob/ea9d2443bd288da0bc4aa1f6dbc66c8db3584b3c/pina/_src/solver/base_solver.py#L126-L134)? 4. The [_EnsembleMixin forward](https://github.com/GiovanniCanali/PINA/blob/ea9d2443bd288da0bc4aa1f6dbc66c8db3584b3c/pina/_src/solver/mixin/ensemble_mixin.py#L5) shuould call the forward super of [_MultiModelMixin](https://github.com/GiovanniCanali/PINA/blob/solver_mixin/pina/_src/solver/mixin/multi_model_mixin.py) and mean the super forward result. 5. How do you envision the integration of weighting, both per residual sample and per conditional components? 6. I would remove typechecker to a safer approach, like [pydantic](https://pydantic.dev/docs/) models.
  1. The loss reduction is stored in self._reduction and removed from self._loss_fn. This separates loss computation into two steps: first, tensor_loss is computed from the residuals; then, the reduction is applied to obtain scalar_loss. See _loss_from_residual and _apply_reduction in BaseSolver.

  2. While your reasoning is correct, I would keep the forward logics separate because merging it looks more error-prone. Also, since this is only a two-line function, I do not think it introduces a meaningful code-duplication issue.

  3. Per-condition weighting is handled by ConditionAggregatorMixin and works as it did in the previous version. Point-wise weights, such as those used in residual-based attention, are handled by dedicated mixins.

  4. I did not get your point, can you explain this better?

I agree on the remaining two points, I'll fix them soon

@GiovanniCanali GiovanniCanali added pr-to-review Label for PR that are ready to been reviewed and removed pr-to-fix Label for PR that needs modification labels Jun 8, 2026
@GiovanniCanali GiovanniCanali linked an issue Jun 8, 2026 that may be closed by this pull request
@GiovanniCanali GiovanniCanali marked this pull request as ready for review June 8, 2026 09:32
@GiovanniCanali GiovanniCanali requested review from a team and dario-coscia as code owners June 8, 2026 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.3 Related to 0.3 release enhancement New feature or request pr-to-review Label for PR that are ready to been reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New structure for solver

2 participants