[PREREQUISITE] Clear reused Matplotlib figures#1081
Conversation
|
@pariterre, this isolated prerequisite fixes the shared Matplotlib failure seen across the first review wave. The six focused plotting tests pass locally; it contains only three changes. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1081 +/- ##
==========================================
- Coverage 77.26% 77.26% -0.01%
==========================================
Files 196 196
Lines 21508 21506 -2
==========================================
- Hits 16618 16616 -2
Misses 4890 4890
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI update: all 18 macOS/Linux/Windows test jobs pass, along with build, project coverage and merge coverage. Only the external codecov/patch threshold is red; there is no functional test failure. @pariterre, this prerequisite is ready to merge so the three Wave 1 branches can be updated cleanly from master. |
pariterre
left a comment
pariterre
left a comment
There was a problem hiding this comment.
@mickaelbegon Is "clear" the flag to keep the same behavior as before the update of matplotlib? If I am not mistaken, the expected behavior here is that the subplots stacks on each other (which I am not 100% sure).
@pariterre reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on mickaelbegon).
|
Since the online callback with TCP, plot_ipopt_outputs and check_conditioning have been unreachable either with |
|
@EveCharbie Does this work with the multiprocess? |
|
@pariterre Yes (on my old env), thanks ! 😅 I don't understand what changed :/ |
|
what I understood is that using clear=True reuses the named Matplotlib window while removing axes and plots left by previous calls. This prevents overlapping subplot grids, avoids unnecessary memory growth, and ensures each diagnostic displays only the current result. |
|
@EveCharbie This is not related to the current changes. It is related to an update of Casadi. Since 3.7.0. (if I recall well), MX/SX can no longer be pickled. This means sending values to the multiprocess backend won't work. This is one of the reasons I pushed the DEFAULT to SERVER at some point. There is probably a fix, but it can be hard to implement (i.e. starting the multiprocess and recreating the full MX/SX tree from scratch in graph side) |
|
Yes, that was my suspicion too. |
|
@mickaelbegon This is my understanding too, but I think the graphs were meant to overlap unless we were relying on a memory leak (which is totally possible). If the graphs are meant to be replaced, then the clear=True is actually fixing a bug we had, but if the graphs are meant to overlap (e.g. with transparency or white spaces filled with previous values), then clear=True will wipe out the previous graphs. |
|
Yes clear=True replacing the graph is the expected behavior and should be fine :) |
3 participants
Summary
Root cause
Recent Matplotlib versions reject
plt.subplots(num=...)when a figure with that name already exists unlessclear=Trueis requested. Parametrized tests therefore failed on their second case, independently of the feature PR under test.Validation
pytest tests/shard1/test__global_plots.py::test_plot_check_conditioning tests/shard1/test__global_plots.py::test_plot_check_conditioning_live tests/shard1/test__global_plots.py::test_plot_ipopt_output_live -qThis is a small CI prerequisite for the first review wave (#1068, #1069 and #1076).
This change is