Skip to content

Conversation

@tlunet
Copy link
Member

@tlunet tlunet commented Jun 23, 2024

After implementing most of the core coefficients generation of pySDC into qmat, here is the associated PR finalizing the switch mentioned in #436.

Main changes :

  • LagrangeApproximation and NodeGenerator classes (and their associated tests) are now in qmat
  • the base CollBase class is now mostly an interface to the Collocation class from qmat
  • the base Sweeper class produces implicit and explicit QDelta matrices using qmat generators
  • additional base pip dependency for pySDC set to qmat>=0.1.8
  • docstring / documentation update to link to qmat documentation when needed

Additional changes :

  • includes complete adaptation to numpy v2, based on the first changes done by @brownbaerchen in his fork. The problematic behavior for the parallelSDC project has been patch by removing testing (and coverage) of the plotting function for preconditioner_playground_MPI. If this PR is accepted, no need to merge Some fixes for the new numpy 2.0 version. #443 anymore as it was already merged in this PR.
  • Sweeper.get_qDelta_implicit and Sweeper.get_qDelta_explicit only have one argument now : qd_type. The previous coll one was simply replaced by self.coll, as no Sweeper instance in pySDC code use another Collocation class as the base underlying collocation for the sweeper. This was a weird mix between OOP and Procedural Programming that does not make sense but rather the code more complex than it should be.
  • complete renaming of the core sub-modules to follow a naming consistent with contribution guidelines, using the refactoring tools from VSCode. Only one issue had to be solved manually for a private class name based attribute in a Hook class (see https://github.com/tlunet/pySDC/commit/32f64bf032119f989c9302c3c077d1cb4fc07de8, I won't even try to understand the idea behind this sort of attribute ...)

@codecov
Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 92.12730% with 47 lines in your changes missing coverage. Please review.

Project coverage is 77.83%. Comparing base (18989d3) to head (97fc219).
Report is 22 commits behind head on master.

Files Patch % Lines
...ySDC/projects/Performance/controller_MPI_scorep.py 0.00% 5 Missing ⚠️
pySDC/core/sweeper.py 97.43% 3 Missing ⚠️
...ns/problem_classes/GrayScott_1D_FEniCS_implicit.py 0.00% 3 Missing ⚠️
pySDC/implementations/sweeper_classes/explicit.py 0.00% 3 Missing ⚠️
...lementations/transfer_classes/BaseTransfer_mass.py 0.00% 3 Missing ⚠️
pySDC/projects/Resilience/dahlquist.py 0.00% 3 Missing ⚠️
...ementations/problem_classes/AllenCahn_2D_FD_gpu.py 0.00% 2 Missing ⚠️
...mentations/problem_classes/AllenCahn_2D_FFT_gpu.py 0.00% 2 Missing ⚠️
...entations/problem_classes/Boussinesq_2D_FD_imex.py 0.00% 2 Missing ⚠️
...tations/problem_classes/HeatEquation_ND_FD_CuPy.py 0.00% 2 Missing ⚠️
... and 14 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
+ Coverage   77.38%   77.83%   +0.44%     
==========================================
  Files         327      332       +5     
  Lines       26085    25840     -245     
==========================================
- Hits        20187    20112      -75     
+ Misses       5898     5728     -170     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pancetta
Copy link
Member

Great work, thanks a lot. Taking the quadrature stuff out of pySDC is a very good idea and I appreciate your work.

@pancetta
Copy link
Member

Not sure why you're obscuring this PR with so much renaming, though.. this makes it much harder to see what actually happens here.

@pancetta
Copy link
Member

Is there a reason why the CI pipeline is back to 40+minutes now? Or is that a GitHub glitch? I see no obvious cause.

@tlunet
Copy link
Member Author

tlunet commented Jun 24, 2024

Not sure why you're obscuring this PR with so much renaming, though.. this makes it much harder to see what actually happens here.

Well, this was a big change anyway, and the naming of the core classes has been confusing me since a long time ... so that's mostly shooting two birds with one stone.

@pancetta
Copy link
Member

Not sure why you're obscuring this PR with so much renaming, though.. this makes it much harder to see what actually happens here.

Well, this was a big change anyway, and the naming of the core classes has been confusing me since a long time ... so that's mostly shooting two birds with one stone.

Understood. As a preference, I think smaller PRs with a clear focus are better, though.

@pancetta
Copy link
Member

And no, this is not stopping me from merging this, of course!

@tlunet
Copy link
Member Author

tlunet commented Jun 24, 2024

And no, this is not stopping me from merging this, of course!

I guessed 😅 ... but don't worry, I'll stick to smaller PR with one dedicated topic from now.

Also in my opinion, it would make sense to drop a new release after the PR is merged.

@pancetta
Copy link
Member

For sure and already on my todo list!

@tlunet
Copy link
Member Author

tlunet commented Jun 24, 2024

Is there a reason why the CI pipeline is back to 40+minutes now? Or is that a GitHub glitch? I see no obvious cause.

Probably a GitHub glitch ... but project testing still take quite some time (especially the parallelSDC, DAE and Resilience). Where are we on testing projects only for one Python version ?

@pancetta
Copy link
Member

Unless this will significantly shorten the CI time, I'd rather have broad coverage.

@tlunet
Copy link
Member Author

tlunet commented Jun 24, 2024

Unless this will significantly shorten the CI time

Probably not, since most are run in parallel ... just significantly less computation ressource used (mostly energy saving)

@pancetta pancetta merged commit c42bb65 into Parallel-in-Time:master Jun 24, 2024
@tlunet tlunet deleted the qmat-switch branch June 24, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants