Skip to content

Solver mixin#804

Draft
GiovanniCanali wants to merge 10 commits into
mathLab:0.3from
GiovanniCanali:solver_mixin
Draft

Solver mixin#804
GiovanniCanali wants to merge 10 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 2 times, most recently from bff3bb7 to ed9f244 Compare May 29, 2026 14:10
@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

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-fix Label for PR that needs modification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants