fix: correct nice=True behavior#12
Merged
Merged
Conversation
Reviewer's GuideAdjusts range_frame behavior so that padding is consistently applied around spine bounds for both nice and non-nice axes, adds regression tests to validate pad behavior with nice=True, and updates README documentation to reflect the new semantics. Flow diagram for updated range_frame padding and nice behaviorflowchart TD
A[start_range_frame] --> B[determine_pad_x_and_pad_y]
B --> C{y_is_numeric?}
C -- yes --> D{y_has_variation?}
C -- no --> J[set_y_bounds_to_y_min_and_y_max]
D -- no --> J
D -- yes --> E{nice_true_for_y?}
E -- yes --> F[compute_y_bound_min_y_bound_max_and_y_ticks_via_nice_tick_bounds]
F --> G[set_yticks_to_y_ticks]
E -- no --> H[set_y_bound_min_to_y_min_and_y_bound_max_to_y_max]
G --> I[compute_y_range_as_y_bound_max_minus_y_bound_min]
H --> I
I --> K[set_ylim_to_y_bound_min_minus_pad_x_times_y_range_and_y_bound_max_plus_pad_x_times_y_range]
J --> L[set_ylim_to_y_min_and_y_max]
B --> M{x_is_numeric?}
M -- yes --> N{ x_has_variation? }
M -- no --> U[set_x_bounds_to_x_min_and_x_max]
N -- no --> U
N -- yes --> O{nice_true_for_x?}
O -- yes --> P[compute_x_bound_min_x_bound_max_and_x_ticks_via_nice_tick_bounds]
P --> Q[set_xticks_to_x_ticks]
O -- no --> R[set_x_bound_min_to_x_min_and_x_bound_max_to_x_max]
Q --> S[compute_x_range_as_x_bound_max_minus_x_bound_min]
R --> S
S --> T[set_xlim_to_x_bound_min_minus_pad_y_times_x_range_and_x_bound_max_plus_pad_y_times_x_range]
U --> V[set_xlim_to_x_min_and_x_max]
K --> W[end_range_frame]
L --> W
T --> W
V --> W
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe pull request makes padding behavior in range_frame consistent across Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The tests asserting padding behavior use a very tight absolute tolerance of 1e-10 on floating point comparisons, which could be fragile across platforms or Matplotlib versions; consider using a slightly looser tolerance or
np.isclosewith relative tolerance instead. - The new
x_range/y_rangeand limits computation logic fornice=Trueandnice=Falseis now duplicated for both axes; consider extracting a small helper to compute(bound_min, bound_max) -> (limit_min, limit_max)to keep the behavior consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests asserting padding behavior use a very tight absolute tolerance of 1e-10 on floating point comparisons, which could be fragile across platforms or Matplotlib versions; consider using a slightly looser tolerance or `np.isclose` with relative tolerance instead.
- The new `x_range`/`y_range` and limits computation logic for `nice=True` and `nice=False` is now duplicated for both axes; consider extracting a small helper to compute `(bound_min, bound_max) -> (limit_min, limit_max)` to keep the behavior consistent and easier to maintain.
## Individual Comments
### Comment 1
<location path="tests/test_plotutils.py" line_range="47-56" />
<code_context>
+ plt.close(fig)
+
+
+def test_range_frame_nice_true_with_pad():
+ """Test that pad is respected even when nice=True."""
+ fig, ax = plt.subplots()
+ x = np.array([1, 2, 3, 4, 5])
+ y = np.array([10, 20, 30, 40, 50])
+
+ range_frame(ax, x, y, pad=0.2, nice=True)
+
+ xlim = ax.get_xlim()
+ ylim = ax.get_ylim()
+
+ # Spine bounds come from nice ticks
+ x_spine = ax.spines["bottom"].get_bounds()
+ y_spine = ax.spines["left"].get_bounds()
+
+ # Limits must extend beyond spine bounds by the pad fraction
+ x_range = x_spine[1] - x_spine[0]
+ y_range = y_spine[1] - y_spine[0]
+
+ assert abs(xlim[0] - (x_spine[0] - 0.2 * x_range)) < 1e-10
+ assert abs(xlim[1] - (x_spine[1] + 0.2 * x_range)) < 1e-10
+ assert abs(ylim[0] - (y_spine[0] - 0.2 * y_range)) < 1e-10
+ assert abs(ylim[1] - (y_spine[1] + 0.2 * y_range)) < 1e-10
+
+ plt.close(fig)
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for `pad_x`/`pad_y` when `nice=True`, not just the symmetric `pad` case.
Since the change also affects the per-axis `pad_x`/`pad_y` handling with `nice=True`, add a test where they differ (e.g. `pad_x=0.0, pad_y=0.2`) and assert that each axis limit uses its own pad value. This will better safeguard the `nice` + per-axis padding behavior against regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+47
to
+56
| def test_range_frame_nice_true_with_pad(): | ||
| """Test that pad is respected even when nice=True.""" | ||
| fig, ax = plt.subplots() | ||
| x = np.array([1, 2, 3, 4, 5]) | ||
| y = np.array([10, 20, 30, 40, 50]) | ||
|
|
||
| range_frame(ax, x, y, pad=0.2, nice=True) | ||
|
|
||
| xlim = ax.get_xlim() | ||
| ylim = ax.get_ylim() |
There was a problem hiding this comment.
suggestion (testing): Add coverage for pad_x/pad_y when nice=True, not just the symmetric pad case.
Since the change also affects the per-axis pad_x/pad_y handling with nice=True, add a test where they differ (e.g. pad_x=0.0, pad_y=0.2) and assert that each axis limit uses its own pad value. This will better safeguard the nice + per-axis padding behavior against regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lama_aesthetics/plotutils.py`:
- Around line 81-83: The sentence claiming "Regardless of `nice`, the `pad` /
`pad_x` / `pad_y` parameters control how far the visible axis limits extend
beyond the spine bounds..." should be narrowed to numeric axes only (or
explicitly state categorical behavior): update the documentation in plotutils.py
near the `nice`, `pad`, `pad_x`, and `pad_y` parameter descriptions so it reads
that these padding parameters apply to numeric (continuous) axes; also add a
brief note that non-numeric/categorical axes bypass this padding (or describe
how categorical spacing is handled) so readers know the exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69c3899a-e683-4ac3-8a14-3399539ad2af
📒 Files selected for processing (1)
src/lama_aesthetics/plotutils.py
Comment on lines
+81
to
+83
| Regardless of `nice`, the `pad` / `pad_x` / `pad_y` parameters | ||
| control how far the visible axis limits extend beyond the spine bounds, | ||
| giving data points breathing room from the spine edges. |
There was a problem hiding this comment.
Clarify scope of the “regardless of nice” statement.
Current wording reads as if padding always applies for all axis types, but non-numeric axes still bypass padding. Please narrow this sentence to numeric axes (or explicitly document the categorical behavior).
✏️ Suggested doc wording
- Regardless of `nice`, the `pad` / `pad_x` / `pad_y` parameters
- control how far the visible axis limits extend beyond the spine bounds,
- giving data points breathing room from the spine edges.
+ For numeric axes, regardless of `nice`, the `pad` / `pad_x` / `pad_y`
+ parameters control how far the visible axis limits extend beyond the
+ spine bounds, giving data points breathing room from the spine edges.
+ For non-numeric axes, limits follow categorical index bounds.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Regardless of `nice`, the `pad` / `pad_x` / `pad_y` parameters | |
| control how far the visible axis limits extend beyond the spine bounds, | |
| giving data points breathing room from the spine edges. | |
| For numeric axes, regardless of `nice`, the `pad` / `pad_x` / `pad_y` | |
| parameters control how far the visible axis limits extend beyond the | |
| spine bounds, giving data points breathing room from the spine edges. | |
| For non-numeric axes, limits follow categorical index bounds. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lama_aesthetics/plotutils.py` around lines 81 - 83, The sentence claiming
"Regardless of `nice`, the `pad` / `pad_x` / `pad_y` parameters control how far
the visible axis limits extend beyond the spine bounds..." should be narrowed to
numeric axes only (or explicitly state categorical behavior): update the
documentation in plotutils.py near the `nice`, `pad`, `pad_x`, and `pad_y`
parameter descriptions so it reads that these padding parameters apply to
numeric (continuous) axes; also add a brief note that non-numeric/categorical
axes bypass this padding (or describe how categorical spacing is handled) so
readers know the exception.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Adjust range_frame axis limit handling so padding is consistently applied relative to spine bounds, and expand tests and documentation to reflect the corrected nice=True behavior.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Documentation
Tests