Skip to content

Conversation

@lucasb-eyer
Copy link

I did try to fix this myself, but failed. So I asked Codex (gpt5-high) and it found the issue and proposed this fix, which I verified works, and I streamlined its code a bit.

Here's what it had to say:

  • mplexporter/mplexporter/exporter.py:6-275 now imports numpy/mpath and routes collections through _prepare_collection_data, which preserves data-space vertices/offsets whenever a non-affine ax.transData branch is
    involved (log scales, semilog, etc.). As a result, fill_between, scatter, and other PathCollections on log axes export with pathcoordinates/offsetcoordinates == 'data', so the rendered polygons obey zooming/panning.
  • mpld3/tests/test_elements.py:76-103 adds regression tests for both fill_between and scatter on logarithmic axes to guard against future regressions in the exported coordinate system.

Here's an example of my app before this fix:
image

And after this fix:
image

I did try to fix this myself, but failed. So I asked Codex (gpt5-high)
and it found the issue and proposed this fix, which I verified works,
and I streamlined its code a bit.

Here's what it had to say:

> - `mplexporter/mplexporter/exporter.py:6-275` now imports `numpy/mpath` and routes collections through `_prepare_collection_data`, which preserves data-space vertices/offsets whenever a non-affine `ax.transData` branch is
>    involved (log scales, semilog, etc.). As a result, `fill_between`, `scatter`, and other `PathCollections` on log axes export with `pathcoordinates/offsetcoordinates == 'data'`, so the rendered polygons obey zooming/panning.
> - `mpld3/tests/test_elements.py:76-103` adds regression tests for both `fill_between` and `scatter` on logarithmic axes to guard against future regressions in the exported coordinate system.
@lucasb-eyer
Copy link
Author

PS: this is the test it's talking about adding to mpld3 repo. I would open a PR there which also moves the submodule, if you agree this is meaningful:

diff --git a/mpld3/tests/test_elements.py b/mpld3/tests/test_elements.py
index 8a89772..dbce6bf 100644
--- a/mpld3/tests/test_elements.py
+++ b/mpld3/tests/test_elements.py
@@ -76,6 +76,29 @@ def test_scatter():
                  [[7.607257743127308, 0.0, 0.0, 7.607257743127308, 0.0, 0.0]])
 
 
+def test_fill_between_logscale():
+    fig, ax = plt.subplots()
+    ax.set_xscale('log')
+    ax.set_yscale('log')
+    x = np.logspace(0, 2, 5)
+    ax.fill_between(x, x, x * 2)
+    rep = fig_to_dict(fig)
+    coll = rep['axes'][0]['collections'][0]
+
+    assert_equal(coll['pathcoordinates'], 'data')
+
+
+def test_scatter_logscale():
+    fig, ax = plt.subplots()
+    ax.set_xscale('log')
+    ax.set_yscale('log')
+    ax.scatter([1, 10], [1, 100])
+    rep = fig_to_dict(fig)
+    coll = rep['axes'][0]['collections'][0]
+
+    assert_equal(coll['offsetcoordinates'], 'data')
+
+
 def test_patch():
     fig, ax = plt.subplots()
     ax.add_patch(plt.Rectangle((0, 0), 1, 2, alpha=0.2, linewidth=2,

@lucasb-eyer
Copy link
Author

I think this might close multiple issues in mpld3 proper, but I am not going to test each of them, of course:

mpld3/mpld3#488
mpld3/mpld3#227
mpld3/mpld3#487

@vladh
Copy link
Member

vladh commented Nov 11, 2025

Thanks for this! This looks good at first sight, but I need to do some more in-depth review, because (1) AI, (2) I want to make sure I actually understand the change well, and (3) the code currently is named to coincide with the original matplotlib function and argument names, which isn't necessary. Will get back to you. 😊

@lucasb-eyer
Copy link
Author

lucasb-eyer commented Nov 11, 2025

  1. Heh, I guessed so which is why I even mentioned it, but just FYI: I did carefully review it, but AI has the whole matplotlib internals in its head and I dont. From old issues, it looks like even Jake had no idea on why the issue exists :)
  2. That's a change I did on top of AI intentionally - you can now easily diff it with the matplotlib funtion to see the actual change (which I did to review it), and if matplotlib ever changes that function, we can easily adopt the changes.

PS: I will have a couple more such AI-assisted fixes coming in the near future as I go through missing features for my use-case. All of them are open issues, and I'll always disclose when I use AI.

@aflaxman
Copy link
Contributor

aflaxman commented Dec 7, 2025

I think this is so cool! AI-assisted coding could be a really good match for this project. I did so (AI-assisted) review of this PR and I can confirm that it works. You can find my notes here.

@vladh
Copy link
Member

vladh commented Dec 7, 2025

Just checking in to say:

  • Hey @aflaxman, great to see you here! Feel free to review and merge things yourself if you'd like.
  • Feel free to send a PR to add contributions from your mpld3 notes to CONTRIBUTING.md and potentially RELEASING.md. In particular, adding uv notes to these documents would be great.
  • I'm trying to make time to review PRs in the next week or so

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