Add sweeper implementation guide without type checking infrastructure#612
Add sweeper implementation guide without type checking infrastructure#612
Conversation
Co-authored-by: pancetta <7158893+pancetta@users.noreply.github.com>
Co-authored-by: pancetta <7158893+pancetta@users.noreply.github.com>
brownbaerchen
left a comment
brownbaerchen
left a comment
There was a problem hiding this comment.
Personally, I am not a fan of type hints. In my opinion, they clutter the code and make it less readable. Python and pySDC are about having the code look similar to the equations. I find that type hints are detracting from this.
In particular, I find it inhibiting for newcomers rather than the other way around. With type hints, you need to know what type something has, which may be some class in the code that you are just starting to learn. Without type hints, you can focus on the patch of code you're working with, without understanding the inputs and outputs fully. I know that this is precisely the goal of type hints. Again, I just don't like it. It doesn't help me understand code and I have an easier time coding without.
But I am aware that plenty of people like type hints. So go ahead with this if you want ;)
Unrelated side remark: Less is more.
docs/contrib/09_type_checking.md
Outdated
|
|
||
| ```toml | ||
| [tool.mypy] | ||
| python_version = "3.10" |
There was a problem hiding this comment.
why 3.10? This will be out of support next year, I think.
pyproject.toml
Outdated
|
|
||
| [tool.mypy] | ||
| # Start with permissive settings for gradual adoption | ||
| python_version = "3.10" |
There was a problem hiding this comment.
This is the place where I would like to see version >=3.13. The other page should maybe point here.
|
I agree. I wanted to see how type hints might look like and I am not happy with what I see. Plus, I don't think this is helping anyone. |
Co-authored-by: pancetta <7158893+pancetta@users.noreply.github.com>
|
Got rid of type hinting. Now left with new sweeper doc strings. I don't know why this was mixed up here, most likely a user fault. |
|
If I may ... I think type hints can be useful sometimes to help documentation (like, one argument is supposed to be a sweeper class). But systematically adding those is indeed too much, so I would recommend using it with care : add them from time to time when it effectively improves documentation. |
brownbaerchen
left a comment
brownbaerchen
left a comment
There was a problem hiding this comment.
I am not a huge fan. I understand the problem that the different kinds of sweeps and their implementations are confusing to newcomers. However, this struggles with the classic challenges of documentation.
My main problem is that some statements are vague, some are slightly false, some are a bit misleading.
The other issue is that this may get out of date. I know that pySDC developments is slowing down, but if somebody changes the code, they will probably not update this.
If we want something like this, we should give it another thorough read and fix all the small details and remember that documentation is lying a lot..
docs/contrib/08_sweeper_guide.md
Outdated
| - `'IE'`: Implicit Euler | ||
| - `'EE'`: Explicit Euler | ||
| - `'LU'`: LU decomposition | ||
| - `'MIN'`: Minimal correction |
There was a problem hiding this comment.
| - `'MIN'`: Minimal correction | |
| - `'MIN'`: Diagonal |
There was a problem hiding this comment.
Fixed in 9078b16 - changed MIN description to "Diagonal"
| - When you want full control over the integration matrix | ||
| - As a base for understanding how SDC works |
There was a problem hiding this comment.
is that not true for all of the sweepers?
There was a problem hiding this comment.
Yes, all sweepers need to implement integrate() and update_nodes(). This section describes the base Sweeper class that all implementations inherit from.
There was a problem hiding this comment.
I meant lines 43 and 44 apply to all sweepers
docs/contrib/08_sweeper_guide.md
Outdated
| **When to Use**: | ||
| - Non-stiff problems where explicit methods are stable | ||
| - Fast computations without implicit solves | ||
| - Problems where CFL conditions are easily satisfied |
There was a problem hiding this comment.
| - Problems where CFL conditions are easily satisfied |
somewhat redundant with the the first point
There was a problem hiding this comment.
Fixed in 9078b16 - removed redundant CFL condition point
docs/contrib/08_sweeper_guide.md
Outdated
| - **MIN**: Minimal correction | ||
| - **MIN3**: Minimal correction variant |
There was a problem hiding this comment.
| - **MIN**: Minimal correction | |
| - **MIN3**: Minimal correction variant | |
| - **MIN-SR-S**: Diagonal, optimized for stiff problems | |
| - **MIN-SR-S**: Diagonal, optimized for non-stiff problems | |
| - **MIN-SR-FLEX**: Diagonal, optimized for stiff problems, coefficients change between iterations |
There was a problem hiding this comment.
Fixed in 9078b16 - added MIN-SR-S (stiff), MIN-SR-NS (non-stiff), and MIN-SR-FLEX (stiff with changing coefficients)
|
|
||
| **When to use:** | ||
| - Non-stiff problems where explicit methods are stable | ||
| - Fast computations without implicit system solves |
There was a problem hiding this comment.
| - Fast computations without implicit system solves |
There was a problem hiding this comment.
Fixed in 9078b16 - removed redundant line about implicit system solves
| Generic implicit sweeper, expecting lower triangular matrix type as input. | ||
|
|
||
| This is the foundational sweeper for standard SDC methods with implicit time integration. | ||
| It uses a single lower triangular integration matrix (QI) to update collocation nodes. |
There was a problem hiding this comment.
| It uses a single lower triangular integration matrix (QI) to update collocation nodes. | |
| It uses a single lower triangular preconditioner (QI) to update collocation nodes. |
There was a problem hiding this comment.
Fixed in 9078b16 - changed "integration matrix" to "preconditioner"
|
|
||
| **When to use:** | ||
| - Standard SDC with implicit time integration | ||
| - When you want full control over the integration matrix type |
There was a problem hiding this comment.
| - When you want full control over the integration matrix type |
There was a problem hiding this comment.
Fixed in 9078b16 - removed redundant bullet point about full control
| QI (numpy.ndarray): Implicit Euler integration matrix for implicit terms | ||
| QE (numpy.ndarray): Explicit Euler integration matrix for explicit terms | ||
| """ |
There was a problem hiding this comment.
| QI (numpy.ndarray): Implicit Euler integration matrix for implicit terms | |
| QE (numpy.ndarray): Explicit Euler integration matrix for explicit terms | |
| """ | |
| QI (numpy.ndarray): Preconditioner for implicit terms | |
| QE (numpy.ndarray): Preconditioner for explicit terms | |
| """ |
There was a problem hiding this comment.
Fixed in 9078b16 - changed "integration matrix" to "preconditioner" in attributes
Co-authored-by: brownbaerchen <39156931+brownbaerchen@users.noreply.github.com>
Address PR review comments on sweeper guide
Addressed feedback from @brownbaerchen on the sweeper documentation guide and code docstrings.
Documentation Changes (docs/contrib/08_sweeper_guide.md) ✅
quad_typeparameter to examplesCode Docstring Changes ✅
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.