tests: Check results of nebulosity estimation when no convergence - #487
tests: Check results of nebulosity estimation when no convergence#487lou-qui wants to merge 9 commits into
Conversation
Signed-off-by: lou-qui <184963772+lou-qui@users.noreply.github.com>
Signed-off-by: lou-qui <184963772+lou-qui@users.noreply.github.com>
| def test_nebulosity__no_solution() -> None: | ||
| pass | ||
| def test_nebulosity__no_solution(caplog: pytest.LogCaptureFixture) -> None: | ||
| with caplog.at_level(logging.WARNING): |
There was a problem hiding this comment.
For now running this tests causes warnings to appear in the logs, it might be useful to prevent that so as not to pollute the logs with useless messages? Do you have any idea how to do that?
There was a problem hiding this comment.
I would prefer verify a warning and not to test the logs.
There was a problem hiding this comment.
I suggest to fill the gap in thermohl itself.
In mechaphlowers we think that logger output is very fragile and should be used minimum possible.
Is it possible to warn in thermohl and catch it in mechaphlowers ? But catching warning code is not easy.
A simpler way could be to return 2 values: the result and a convergence flag. It passes the convergence information allowing to the caller to warn or raise with a simple if else. This is my suggestion.
There was a problem hiding this comment.
I added a convergence flag in thermohl (see phlowers/thermohl#169). Now waiting for a new release of thermohl, then I can adapt the code here in mechaphlowers.
There was a problem hiding this comment.
New version of thermohl released and used in mechaphlowers!
There was a problem hiding this comment.
Pull request overview
Adds coverage for ThermalEngine.nebulosity() behavior when the underlying nebulosity estimation does not converge, and updates the API docstring to describe the expected boundary behavior.
Changes:
- Replaces the previously skipped
test_nebulosity__no_solutionwith assertions for the non-convergent case (including warning capture). - Adds expectations that the result clamps to boundary nebulosities (
0/8) when no solution is found. - Updates
ThermalEngine.nebulosity()docstring to document the clamp behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/core/models/cable/test_thermal.py | Adds an active regression test for the “no convergence” nebulosity estimation path and checks returned boundary values + warning logging. |
| src/mechaphlowers/core/models/cable/thermal.py | Documents the clamp-to-boundaries behavior when no nebulosity yields the requested radiation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| def test_nebulosity__no_solution() -> None: | ||
| pass | ||
| def test_nebulosity__no_solution(caplog: pytest.LogCaptureFixture) -> None: | ||
| with caplog.at_level(logging.WARNING): |
There was a problem hiding this comment.
I would prefer verify a warning and not to test the logs.
| def test_nebulosity__no_solution() -> None: | ||
| pass | ||
| def test_nebulosity__no_solution(caplog: pytest.LogCaptureFixture) -> None: | ||
| with caplog.at_level(logging.WARNING): |
There was a problem hiding this comment.
I suggest to fill the gap in thermohl itself.
In mechaphlowers we think that logger output is very fragile and should be used minimum possible.
Is it possible to warn in thermohl and catch it in mechaphlowers ? But catching warning code is not easy.
A simpler way could be to return 2 values: the result and a convergence flag. It passes the convergence information allowing to the caller to warn or raise with a simple if else. This is my suggestion.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: lou-qui <184963772+lou-qui@users.noreply.github.com>
Signed-off-by: lou-qui <184963772+lou-qui@users.noreply.github.com>
Signed-off-by: lou-qui <184963772+lou-qui@users.noreply.github.com>
|
Please check if the PR fulfills these requirements