Skip to content

Conversation

@GiovanniCanali
Copy link
Collaborator

@GiovanniCanali GiovanniCanali commented Mar 29, 2025

Fixes #525.

To do:

  • Remove unneccessary checks;
  • Remove multidimensional requirement for div;
  • Fix advection;
  • Implement divgrad method for laplacian;
  • Update documentation;
  • Add tests;
  • Add fast implementations of differential operators.

@GiovanniCanali GiovanniCanali added enhancement New feature or request pr-to-fix Label for PR that needs modification labels Mar 29, 2025
@GiovanniCanali GiovanniCanali self-assigned this Mar 29, 2025
@GiovanniCanali GiovanniCanali linked an issue Mar 29, 2025 that may be closed by this pull request
@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 Mar 31, 2025
@GiovanniCanali GiovanniCanali marked this pull request as ready for review March 31, 2025 09:04
@GiovanniCanali
Copy link
Collaborator Author

GiovanniCanali commented Mar 31, 2025

Hi @ndem0, @dario-coscia, @FilippoOlivo,

Tests for python 3.8 are failing in several different modules, not related to this PR. Since this version is no longer supported, I suggest to remove them.


Preliminary results in terms of speedup:

  • grad: 15% - 20% faster.
  • div: 20% - 25% faster.
  • laplacian (std method): up to 40% faster.
  • laplacian (divgrad method): much slower than std. Not available before.

Reported results are a qualitative average derived from the combination of these values:

  • device: [cpu, cuda]
  • number of points: [1, 10, 100, 1000, 10000, 100000]
  • number of input components: [1, 2, 3, 4, 5]
  • number of output components: [1, 2, 3, 4, 5]

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.

Thank you for the PR, really impressive the speedup. Since we are working on this I also suggested further improvements which could speed up even more! We need however to restructure a bit..

Idea:

  1. Have fast versions of operators where no checks are performed (nothing). These are exact copies of the ones we already have but with no checks. Why? Sometimes when calling recursively grad we do the same check (but we actually need to do the check only once at the beginning!). We can also think of doing a separate module called fast_operator.py where we put these operators. Of course, we need to raise a warning when imported saying checks are not going to be performed and also write it in the doc.

  2. Rewrite our operators by calling the fast versions, avoiding multiple repeated checks

@GiovanniCanali
Copy link
Collaborator Author

Thanks to @dario-coscia for spotting a bug in laplacian and helping writing the new tests, which should now be more complete.

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.

Just a minor change! The tests look good, and I think we are done besides that comment on advection

@dario-coscia dario-coscia merged commit 530ade7 into mathLab:dev Apr 2, 2025
17 of 19 checks passed
@GiovanniCanali GiovanniCanali deleted the diff_ops branch April 2, 2025 14:37
@GiovanniCanali GiovanniCanali restored the diff_ops branch April 2, 2025 15:10
@GiovanniCanali GiovanniCanali deleted the diff_ops branch April 3, 2025 08:25
dario-coscia added a commit that referenced this pull request Apr 4, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
dario-coscia added a commit that referenced this pull request Apr 8, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
dario-coscia added a commit that referenced this pull request Apr 14, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
FilippoOlivo pushed a commit to FilippoOlivo/PINA that referenced this pull request Apr 17, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
dario-coscia added a commit that referenced this pull request Apr 17, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
dario-coscia added a commit that referenced this pull request Apr 23, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
GiovanniCanali added a commit to GiovanniCanali/PINA that referenced this pull request Dec 2, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
GiovanniCanali added a commit to GiovanniCanali/PINA that referenced this pull request Dec 2, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
GiovanniCanali added a commit to GiovanniCanali/PINA that referenced this pull request Dec 2, 2025
* Improve grad logic and fix issues

* Add operators' fast versions

* Fix bug in laplacian + new tests + restructuring

Co-authored-by: Dario Coscia <[email protected]>

* fix advection bug

---------

Co-authored-by: Dario Coscia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Improve the logic of differential operators

3 participants