Skip to content

Conversation

GiovanniCanali
Copy link
Collaborator

Description

This PR fixes #644

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 Sep 16, 2025
@GiovanniCanali GiovanniCanali added the enhancement New feature or request label Sep 16, 2025
@GiovanniCanali GiovanniCanali added low priority Low priority fix pr-to-review Label for PR that are ready to been reviewed labels Sep 16, 2025
@GiovanniCanali GiovanniCanali marked this pull request as ready for review September 16, 2025 14:23
@dario-coscia
Copy link
Collaborator

I would not remove Laplace, maybe add a fix laplacian but I would keep the Laplace (as an equation). In principle we could add Burgers, Advection, ...

@GiovanniCanali
Copy link
Collaborator Author

GiovanniCanali commented Sep 16, 2025

I would not remove Laplace, maybe add a fix laplacian but I would keep the Laplace (as an equation). In principle we could add Burgers, Advection, ...

The FixedLaplacian class allows to do the same thing as Laplace by setting value to 0. The class was renamed for consistency with other classes within the same file.

Many of the mentioned equations are inside the problem definitions in the problem zoo. Do we want to move them all in the equation factory and import them for the problem zoo classes definitions?

@GiovanniCanali
Copy link
Collaborator Author

Will leave Laplace class for back compatibility. It will simply inherit from FixedLaplacian with value set to 0.

@GiovanniCanali GiovanniCanali force-pushed the equation_factory branch 2 times, most recently from 51782a3 to 2de792c Compare September 17, 2025 13:27
@GiovanniCanali
Copy link
Collaborator Author

GiovanniCanali commented Sep 17, 2025

Hi @dario-coscia, @FilippoOlivo,
Since many files were touched, let me summarize changes here:

  • Added FixedLaplacian class. Now, Laplace simply inherits from it and sets value to 0.
  • Moved all the equations defined in the problems of problem/zoo to equation_factory.py. Some of them are now available in a more general form (for instance, AdvectionEquation). Nothing changes with respect to the problem definition: the original equations have been enforced in the problem definitions with a consistent choice of parameters.
  • Added unit tests for equation_factory.py

Note that these changes have no impact on the user, as nothing "importable" has been modified in its behaviour.

Copy link
Collaborator

@dario-coscia dario-coscia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @GiovanniCanali! Thanks for the PR, to me it looks great and more structured as so. I made one minor comment for the tutorial file.

It would be great to also add the mathematical description of the PDE in Poisson, Laplace, Advection, ect. Maybe we can do this in the doc?

@GiovanniCanali
Copy link
Collaborator Author

Hi @GiovanniCanali! Thanks for the PR, to me it looks great and more structured as so. I made one minor comment for the tutorial file.

It would be great to also add the mathematical description of the PDE in Poisson, Laplace, Advection, ect. Maybe we can do this in the doc?

Hi @dario-coscia,
Following your suggestions, I have restored tutorial12/tutorial.ipynb and changed the classes' names from ***Equation to ***. Also, the mathematical descriptions of the equations were already included in the respective docstrings.

@FilippoOlivo FilippoOlivo merged commit 4e37468 into mathLab:dev Sep 22, 2025
17 of 19 checks passed
@GiovanniCanali GiovanniCanali deleted the equation_factory branch September 22, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request low priority Low priority fix 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.

Add the possibility to enforce a fixed laplacian + equation factory tests

3 participants