Skip to content

Solver mixin#804

Merged
dario-coscia merged 3 commits into
mathLab:0.3from
GiovanniCanali:solver_mixin
Jun 8, 2026
Merged

Solver mixin#804
dario-coscia merged 3 commits into
mathLab:0.3from
GiovanniCanali:solver_mixin

Conversation

@GiovanniCanali

@GiovanniCanali GiovanniCanali commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR fixes #628 #782

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

@dario-coscia dario-coscia 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.

Stellar update!!!!

@dario-coscia dario-coscia merged commit 9c6b724 into mathLab:0.3 Jun 8, 2026
17 of 19 checks passed
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

3 participants