| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
Looks like a really nice addition to me and makes sense for how to handle it.
There are a lot of images here for all the different plot styles, some with a lot of lines/ticks on them as well, do we need all artist types to cover these cases or could we reduce it down to just a few of the image tests which would cover all of the capability?
Sorry, something went wrong.
I think we do need all artists exercised, but I consolidated them down into a single test image instead of 7. |
Sorry, something went wrong.
|
I think this is good to go, this is definitely a nice update to the 3d suite.
There are pros and cons to that because now it is a much larger image file and if one artist gets changed/updated you need to regenerate the entire image file. It might have actually been a similar size before just seven separate tests, and if so, I'd probably suggest we go with more individual images instead. I don't have a major preference either way. |
Sorry, something went wrong.
This fixes a long-standing issue where 3D plots ignored scale transforms. The methods set_xscale(), set_yscale(), set_zscale() existed but the transforms were never applied to data or coordinates. Key changes: - axes3d.py: Add _get_scaled_limits() to transform axis limits through scale transforms. Modify get_proj() to use scaled limits for world transformation. Override _update_transScale() to use identity transforms since 3D projection handles scales internally. Update autoscale_view() and _set_lim3d() to apply margins in transformed space. - art3d.py: Add _apply_scale_transforms() utility function. Update all do_3d_projection() methods to apply scale transforms before projection. - axis3d.py: Update _get_coord_info() to return scaled-space bounds. Modify _draw_ticks() to transform tick locations to scaled space. Update draw() and draw_grid() for proper coordinate handling. The fix ensures that: - Data coordinates are transformed through scale transforms before projection (e.g., log10 for log scale) - World transformation matrix maps scaled coordinates to unit cube - Axis ticks, labels, and grid lines position correctly - 2D display transforms remain linear (no double-transformation) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Baseline images regen Baseline images regen
|
Congratulations on fixing a 3-digit bug with a 5-digit PR. |
Sorry, something went wrong.
|
This has been my "white whale" bug for years, super happy to see it merged! Thanks both for reviewing. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR summary
This fixes a long-standing issue where 3D plots ignored scale transforms. The methods set_xscale(), set_yscale(), set_zscale() existed but the transforms were never applied to data or coordinates. There are quite a few new test images, but I think it's necessary to cover all the new behavior.
Closes #209 - a 15 year old bug and matplotlib's oldest open issue!
Out of scope here is the addition of semilogx, semilogxy, etc. helper functions. If we want those it will need to be in a follow-on PR.
Example usage:
Notes on generative AI usage
Details This issue has been sitting in my to-solve backlog for a few years, and on a whim I decided to have Claude Code take a crack at it. I'm blown away by the results - the first commit in this PR which substantially solved the issue was completely written by Opus 4.5 over the course of only about 1 hour with minimal prompting. I left Claude authorship on the commit.My own domain expertise in the 3D section of the codebase definitely helped frame the question and set the direction, but I had basically no experience with the scale transform logic or where all the transform gaps in the 3D code might be. I had already spent a day on this back in 2023, and I expect this would have taken me several more days to learn the right sections of the codebase and implement on my own. In particular, needing to overwrite _update_transScale to an identity transform was a tricky insight that I think would have taken a long time to identify.
The workflow I used, as a case study:
Initial claude code prompt in plan mode:
CC's plan:
# Plan: Fix 3D Log Scale Support (Issue #209) ## Problem Summary 3D plots completely ignore scale transforms (log, symlog, etc.). The methods `set_xscale()`, `set_yscale()`, `set_zscale()` exist but the transforms are never applied to data or coordinates. The root cause is that the 3D pipeline assumes linear transformations everywhere. **Current broken flow:** \``` Raw data → world_transformation(raw_limits) → view → projection → 2D ↑ Scale transforms NEVER applied \``` **Required flow:** \``` Raw data → Scale Transform → Scaled data → world_transformation(scaled_limits) → view → projection → 2D \``` ## Key Concept: World vs Data Coordinates - **Data coordinates**: Raw values as user provides (e.g., `[1, 10, 100, 1000]`) - **Scaled coordinates**: After scale transform (e.g., `[0, 1, 2, 3]` for log10) - **World coordinates**: Normalized to projection-ready space ## Implementation Phases ### Phase 1: Core Infrastructure in axes3d.py **File:** `lib/mpl_toolkits/mplot3d/axes3d.py` 1. **Add helper methods** to get scale transforms: \```python def _get_scale_transforms(self): """Return (x_transform, y_transform, z_transform).""" return (self.xaxis.get_transform(), self.yaxis.get_transform(), self.zaxis.get_transform()) \``` 2. **Modify `get_proj()` (lines 1212-1269)** to transform limits through scales: - Before calling `world_transformation()`, apply scale transforms to limit values - Transform `[xmin, xmax]`, `[ymin, ymax]`, `[zmin, zmax]` through their respective scale transforms - Pass transformed limits to `world_transformation()` 3. **Update docstrings** for `set_xscale`, `set_yscale`, `set_zscale` to remove the warning about non-linear scales yielding nonsensical results ### Phase 2: Scale Transform Helper in art3d.py **File:** `lib/mpl_toolkits/mplot3d/art3d.py` 1. **Add utility function** at module level: \```python def _apply_scale_transforms(xs, ys, zs, axes): """Apply scale transforms to 3D coordinates.""" x_trans = axes.xaxis.get_transform() y_trans = axes.yaxis.get_transform() z_trans = axes.zaxis.get_transform() # Transform each dimension, handle masked arrays return xs_scaled, ys_scaled, zs_scaled \``` 2. **Update `_viewlim_mask()`** to compare in scaled space ### Phase 3: Update All Artist do_3d_projection() Methods **File:** `lib/mpl_toolkits/mplot3d/art3d.py` Apply scale transforms before projection in each class: | Class | Method | Line (approx) | |-------|--------|---------------| | `Line3D` | `draw()` | ~270 | | `Line3DCollection` | `do_3d_projection()` | 494-524 | | `Patch3D` | `do_3d_projection()` | ~570 | | `PathPatch3D` | `do_3d_projection()` | ~620 | | `Patch3DCollection` | `do_3d_projection()` | ~730 | | `Path3DCollection` | `do_3d_projection()` | ~800 | | `Poly3DCollection` | `do_3d_projection()` | ~1334 | | `Text3D` | `draw()` | ~360 | **Pattern for each:** \```python def do_3d_projection(self): # Existing: get raw coordinates segments = self._segments3d # NEW: Apply scale transforms segments_scaled = _apply_scale_transforms( segments[..., 0], segments[..., 1], segments[..., 2], self.axes) # Existing: project (now using scaled coordinates) xyzs = proj3d._proj_transform_vectors(segments_scaled, self.axes.M) \``` ### Phase 4: Fix Axis Drawing in axis3d.py **File:** `lib/mpl_toolkits/mplot3d/axis3d.py` 1. **Modify `_draw_ticks()` (lines 460-478)**: - Tick locations from `tick.get_loc()` are in data space - Apply scale transform before calling `proj3d.proj_transform()` 2. **Modify `draw_grid()`**: - Grid line positions need scale transformation 3. **Modify `_get_coord_info()`**: - Bounds should be in scaled space for proper cube drawing ### Phase 5: Autoscaling and Limits **File:** `lib/mpl_toolkits/mplot3d/axes3d.py` 1. **Update `autoscale_view()`**: Apply scale's `limit_range_for_scale()` to ensure limits are valid for the scale type (e.g., positive for log) 2. **Ensure limit setters** (`set_xlim3d`, etc.) validate limits against scale constraints ### Phase 6: Inverse Transforms for Interaction **File:** `lib/mpl_toolkits/mplot3d/axes3d.py` 1. **Update `format_coord()`**: Convert from scaled space back to data space for display 2. **Update any mouse picking code** that converts screen → data coordinates ## Critical Files to Modify | File | Changes | |------|---------| | `lib/mpl_toolkits/mplot3d/axes3d.py` | `get_proj()`, helper methods, autoscaling, docstrings | | `lib/mpl_toolkits/mplot3d/art3d.py` | All `do_3d_projection()` methods, new helper function | | `lib/mpl_toolkits/mplot3d/axis3d.py` | `_draw_ticks()`, `draw_grid()`, `_get_coord_info()` | ## Implementation Order 1. Add helper functions in `axes3d.py` and `art3d.py` 2. Modify `get_proj()` to use scaled limits 3. Fix axis3d tick/grid drawing 4. Fix all artist do_3d_projection() methods (one at a time, testing each) 5. Fix autoscaling 6. Fix inverse transforms 7. Update docstrings ## Verification Plan ### Test Script \```python import matplotlib.pyplot as plt import numpy as np fig = plt.figure() ax = fig.add_subplot(projection='3d') # Data spanning several orders of magnitude x = np.array([1, 10, 100, 1000]) y = np.array([1, 10, 100, 1000]) z = np.array([1, 10, 100, 1000]) ax.set_xscale('log') ax.set_yscale('log') ax.set_zscale('log') ax.scatter(x, y, z) plt.show() \``` **Expected result:** Points should appear evenly spaced (since log10([1,10,100,1000]) = [0,1,2,3]) ### Regression Testing Run existing 3D test suite to ensure linear scales still work: \```bash python -m pytest lib/mpl_toolkits/mplot3d/tests/ -v \``` ### Visual Tests to Create 1. Log scale on single axis 2. Log scale on all axes 3. Symlog scale 4. Mixed scales (log on x, linear on y, symlog on z) 5. Surface plots with log scales 6. Bar3d with log scales ## Notes - Performance: For linear scales (default), the transform is `IdentityTransform` which should be essentially a no-op - Backwards compatibility: Linear scale behavior should be unchanged - The 2D implementation in `axes/_base.py` (lines 910-944) serves as reference for how transforms should composeI accepted the plan without edits, and CC started working in agent mode. After each iteration I would check the output figure, and send essentially the same message:
After the 4th or 5th iteration things looked like they were working as expected, and that result is what's in the first commit: b73427f
The follow-on commits took half a day, and were more much more manual to ensure correct behavior and test coverage.
PR checklist