feat: add the dark plotting style + font installation#8
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds bundled CMU Sans Serif font registration and helpers, a new dark Matplotlib style, enhanced range_frame behavior (per-axis padding and "nice" bounds), figure decomposition utilities to emit one figure per labeled artist, packaging updates to include fonts, expanded README, and comprehensive tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
4-4: Badge shows unsupported Python versions.The badge indicates support for Python 3.9 and 3.10, but
pyproject.tomlnow requires>=3.11. Update the badge to reflect the new minimum version.-[](https://github.com/lamalab-org/lama-aesthetics/blob/main/pyproject.toml) +[](https://github.com/lamalab-org/lama-aesthetics/blob/main/pyproject.toml)
🧹 Nitpick comments (3)
src/lama_aesthetics/styles/lamalab_dark.mplstyle (2)
1-5: Remove orphaned comments.Lines 1 and 4 contain commented-out color cycle remnants that appear to be development artifacts. Consider removing them for cleaner configuration.
-# '000000', axes.prop_cycle : cycler('color', ['0C5DA5', '00B945', 'FF9500', 'FF2C00', '845B97', '474747', '9e9e9e', "9A607F"]) -#axes.prop_cycle : cycler('color', ["DB444B", "006BA2", "3EBCD2", "379A8B", "EBB434", "#B4BA39", "#9A607F", '#9e9e9e', "#D1B07C"]) -
2-2: Consider consistent color format.The color values mix quote styles (single vs double) and some omit the
#prefix. While Matplotlib accepts both, consistency improves readability.src/lama_aesthetics/aesthetics.py (1)
76-79: Use consistentimportlib.resourcesAPI.
importlib.resources.path()is deprecated since Python 3.11. Since you already use the modern pattern inregister_fonts(), apply the same here for consistency.♻️ Suggested fix
- # Get the file contents as a string - # This will only work for Python 3.7 and later - with importlib.resources.path("lama_aesthetics.styles", style_file) as style_path: + with importlib.resources.as_file(importlib.resources.files("lama_aesthetics.styles").joinpath(style_file)) as style_path: plt.style.use(style_path)
This was
linked to
issues
Mar 14, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
4-4:⚠️ Potential issue | 🟡 MinorUpdate the Python version badge in README.md to match pyproject.toml requirements.
The badge currently shows Python 3.9-3.13 support, but
pyproject.tomlspecifiesrequires-python = ">=3.11". Update the badge to display only Python 3.11, 3.12, and 3.13.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 4, Update the Python version badge in README.md so it matches pyproject.toml's requires-python = ">=3.11": replace the current badge text segment "python-3.9_|_3.10_|_3.11_|_3.12_|_3.13" with "python-3.11_|_3.12_|_3.13" (keeping the same badge URL/parameters and link target), and verify the visible label and query parameters reflect only 3.11, 3.12 and 3.13.
🧹 Nitpick comments (5)
src/lama_aesthetics/plotutils.py (4)
225-259: Consider documenting figure cleanup responsibility.The function creates new
Figureobjects that the caller must close to avoid memory leaks. The docstring example showssavefig()but omitsplt.close(). Consider adding a note or extending the example.📝 Suggested docstring addition
Example:: fig, ax = plt.subplots() ax.plot([0, 1], [0, 1], label="Linear") ax.plot([0, 1], [0, 2], label="Steep") parts = decompose_figure(fig) for label, part_fig in parts: part_fig.savefig(f"{label}.png") + plt.close(part_fig) # Close to free memory + + Note: + The caller is responsible for closing the returned figures + when they are no longer needed. """🤖 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 225 - 259, The docstring for decompose_figure creates new matplotlib.Figure objects but doesn't tell callers they must close them; update the decompose_figure docstring and example to note that each returned Figure should be closed to avoid memory leaks (e.g., call plt.close(part_fig) or use a context/try/finally), and amend the example to show closing each saved part_fig after savefig so callers know they are responsible for cleanup.
175-191: Potential IndexError if scatter has no colors/sizes defined.Lines 186-188 access index
[0]when the array length is 1. While matplotlib typically provides defaults, ifget_facecolor(),get_edgecolor(), orget_sizes()return empty arrays, this would raiseIndexError.🛡️ Defensive fix
facecolors = source_coll.get_facecolor() edgecolors = source_coll.get_edgecolor() sizes = source_coll.get_sizes() + + fc = facecolors if len(facecolors) > 1 else (facecolors[0] if len(facecolors) else None) + ec = edgecolors if len(edgecolors) > 1 else (edgecolors[0] if len(edgecolors) else None) + s = sizes if len(sizes) > 1 else (sizes[0] if len(sizes) else None) + target_ax.scatter( offsets[:, 0], offsets[:, 1], - color=facecolors if len(facecolors) > 1 else facecolors[0], - edgecolors=edgecolors if len(edgecolors) > 1 else edgecolors[0], - s=sizes if len(sizes) > 1 else sizes[0], + color=fc, + edgecolors=ec, + s=s, alpha=source_coll.get_alpha(), label=label, )🤖 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 175 - 191, _replay_scatter can raise IndexError when source_coll.get_facecolor(), get_edgecolor(), or get_sizes() return empty arrays; update _replay_scatter to defensively handle empty results by checking length before indexing and providing safe fallbacks (e.g., use None or sensible matplotlib defaults) instead of directly using [0]; specifically guard the expressions that compute color=..., edgecolors=..., and s=... (currently using facecolors[0], edgecolors[0], sizes[0]) so they use the full array if len>1, the first element if len==1, and a safe default if len==0.
153-154: Grid state copying only checks x-axis gridlines.The current implementation only copies x-axis grid visibility. If the source has y-axis grid enabled independently, this won't be preserved. Consider checking both axes or using
ax.xaxis.get_visible()patterns if full grid fidelity is needed.🤖 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 153 - 154, The code only inspects source_ax.xaxis.get_gridlines() and thus loses y-axis grid state; update the copy so you query both source_ax.xaxis.get_gridlines() and source_ax.yaxis.get_gridlines(), determine visibility for each axis independently, and call target_ax.grid(visible_x, axis='x') and target_ax.grid(visible_y, axis='y') (or equivalent per-axis grid setters) to preserve both x and y grid visibility when copying from source_ax to target_ax.
148-151: Avoid using ternary expression as a statement.Line 150 uses a conditional expression for its side effect, which is non-idiomatic and harder to read. Additionally,
get_bounds()can return(0.0, 1.0)as a default (truthy), so the condition may not behave as expected for "unset" bounds.♻️ Proposed fix
for spine_name in ("top", "bottom", "left", "right"): target_ax.spines[spine_name].set_visible(source_ax.spines[spine_name].get_visible()) - target_ax.spines[spine_name].set_bounds(*source_ax.spines[spine_name].get_bounds()) if source_ax.spines[spine_name].get_bounds() else None + bounds = source_ax.spines[spine_name].get_bounds() + if bounds: + target_ax.spines[spine_name].set_bounds(*bounds) target_ax.spines[spine_name].set_position(source_ax.spines[spine_name].get_position())🤖 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 148 - 151, The loop at spine handling uses a conditional expression for side effects and calls get_bounds() inline; replace the ternary with an explicit check: inside the for loop in the spine-copying block (the lines that reference target_ax.spines[spine_name] and source_ax.spines[spine_name]), call bounds = source_ax.spines[spine_name].get_bounds() and then if bounds is not None: target_ax.spines[spine_name].set_bounds(*bounds); keep the existing set_visible and set_position calls unchanged so the code is clear and avoids using a conditional expression as a statement.tests/test_plotutils.py (1)
1-4: Consider adding a test for the public re-export path.Tests import directly from
lama_aesthetics.plotutils, but users will likely import fromlama_aestheticsdirectly. Consider adding at least one test that verifiesfrom lama_aesthetics import decompose_figureworks correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_plotutils.py` around lines 1 - 4, Add a test that verifies the package-level re-export works by importing decompose_figure (and optionally one other symbol) via "from lama_aesthetics import decompose_figure" and exercising it the same way existing tests do (e.g., create a small matplotlib Figure/axes, call decompose_figure and assert expected outputs or types). Ensure the new test asserts the imported symbol is callable and that its behavior matches the direct import from plotutils (e.g., returns the same structure or types), so the public re-export path is covered.
🤖 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 137-141: The try/except around target_ax.set_xticklabels and
set_yticklabels is swallowing all errors; replace the bare "except Exception:
pass" with catching the specific exceptions that can occur (e.g., ValueError,
IndexError, AttributeError) and surface them via logging or warnings (e.g., call
logging.getLogger(__name__).warning(...) or warnings.warn(...)) so failures
copying tick labels are recorded; update the block around
target_ax.set_xticklabels([t.get_text() for t in source_ax.get_xticklabels()])
and target_ax.set_yticklabels([t.get_text() for t in
source_ax.get_yticklabels()]) accordingly.
- Around line 24-37: The pad_x and pad_y parameter names are counterintuitive
because pad_x is currently applied to ax.set_ylim and pad_y to ax.set_xlim;
update the function so pad_x controls x-axis padding and pad_y controls y-axis
padding by swapping the arguments passed to ax.set_xlim and ax.set_ylim (or
alternatively rename the parameters and update the docstring/tests accordingly);
specifically modify the code using pad_x, pad_y, ax.set_xlim, ax.set_ylim, x,
and y so that x_min/x_max use pad_x and y_min/y_max use pad_y and adjust the
docstring and any tests that depend on the previous semantics.
---
Outside diff comments:
In `@README.md`:
- Line 4: Update the Python version badge in README.md so it matches
pyproject.toml's requires-python = ">=3.11": replace the current badge text
segment "python-3.9_|_3.10_|_3.11_|_3.12_|_3.13" with
"python-3.11_|_3.12_|_3.13" (keeping the same badge URL/parameters and link
target), and verify the visible label and query parameters reflect only 3.11,
3.12 and 3.13.
---
Nitpick comments:
In `@src/lama_aesthetics/plotutils.py`:
- Around line 225-259: The docstring for decompose_figure creates new
matplotlib.Figure objects but doesn't tell callers they must close them; update
the decompose_figure docstring and example to note that each returned Figure
should be closed to avoid memory leaks (e.g., call plt.close(part_fig) or use a
context/try/finally), and amend the example to show closing each saved part_fig
after savefig so callers know they are responsible for cleanup.
- Around line 175-191: _replay_scatter can raise IndexError when
source_coll.get_facecolor(), get_edgecolor(), or get_sizes() return empty
arrays; update _replay_scatter to defensively handle empty results by checking
length before indexing and providing safe fallbacks (e.g., use None or sensible
matplotlib defaults) instead of directly using [0]; specifically guard the
expressions that compute color=..., edgecolors=..., and s=... (currently using
facecolors[0], edgecolors[0], sizes[0]) so they use the full array if len>1, the
first element if len==1, and a safe default if len==0.
- Around line 153-154: The code only inspects source_ax.xaxis.get_gridlines()
and thus loses y-axis grid state; update the copy so you query both
source_ax.xaxis.get_gridlines() and source_ax.yaxis.get_gridlines(), determine
visibility for each axis independently, and call target_ax.grid(visible_x,
axis='x') and target_ax.grid(visible_y, axis='y') (or equivalent per-axis grid
setters) to preserve both x and y grid visibility when copying from source_ax to
target_ax.
- Around line 148-151: The loop at spine handling uses a conditional expression
for side effects and calls get_bounds() inline; replace the ternary with an
explicit check: inside the for loop in the spine-copying block (the lines that
reference target_ax.spines[spine_name] and source_ax.spines[spine_name]), call
bounds = source_ax.spines[spine_name].get_bounds() and then if bounds is not
None: target_ax.spines[spine_name].set_bounds(*bounds); keep the existing
set_visible and set_position calls unchanged so the code is clear and avoids
using a conditional expression as a statement.
In `@tests/test_plotutils.py`:
- Around line 1-4: Add a test that verifies the package-level re-export works by
importing decompose_figure (and optionally one other symbol) via "from
lama_aesthetics import decompose_figure" and exercising it the same way existing
tests do (e.g., create a small matplotlib Figure/axes, call decompose_figure and
assert expected outputs or types). Ensure the new test asserts the imported
symbol is callable and that its behavior matches the direct import from
plotutils (e.g., returns the same structure or types), so the public re-export
path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd95924b-6e10-40ca-a24a-700a9fcab7d1
📒 Files selected for processing (4)
README.mdsrc/lama_aesthetics/__init__.pysrc/lama_aesthetics/plotutils.pytests/test_plotutils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lama_aesthetics/init.py
Comment on lines
+24
to
+37
| pad: The default padding factor applied to both axes. | ||
| pad_x: Padding near the x-axis (vertical direction). Overrides ``pad`` when set. | ||
| pad_y: Padding near the y-axis (horizontal direction). Overrides ``pad`` when set. | ||
| """ | ||
| if pad_x is None: | ||
| pad_x = pad | ||
| if pad_y is None: | ||
| pad_y = pad | ||
|
|
||
| y_min, y_max = y.min(), y.max() | ||
| x_min, x_max = x.min(), x.max() | ||
|
|
||
| ax.set_ylim(y_min - pad * (y_max - y_min), y_max + pad * (y_max - y_min)) | ||
| ax.set_xlim(x_min - pad * (x_max - x_min), x_max + pad * (x_max - x_min)) | ||
| ax.set_ylim(y_min - pad_x * (y_max - y_min), y_max + pad_x * (y_max - y_min)) | ||
| ax.set_xlim(x_min - pad_y * (x_max - x_min), x_max + pad_y * (x_max - x_min)) |
There was a problem hiding this comment.
Confusing parameter naming: pad_x affects y-limits and pad_y affects x-limits.
The naming is counterintuitive. Users would naturally expect pad_x to control x-axis padding and pad_y to control y-axis padding. The current implementation does the opposite:
pad_x→ applied toset_ylim(line 36)pad_y→ applied toset_xlim(line 37)
Consider renaming to match the axis they affect, or clarify the docstring more prominently if "near the x-axis" semantics are intentional.
💡 Suggested fix: rename parameters to match affected axis
-def range_frame(ax, x, y, pad=0.1, pad_x=None, pad_y=None):
+def range_frame(ax, x, y, pad=0.1, pad_x=None, pad_y=None):
"""
...
- pad_x: Padding near the x-axis (vertical direction). Overrides ``pad`` when set.
- pad_y: Padding near the y-axis (horizontal direction). Overrides ``pad`` when set.
+ pad_x: Padding applied to x-axis limits (horizontal direction). Overrides ``pad`` when set.
+ pad_y: Padding applied to y-axis limits (vertical direction). Overrides ``pad`` when set.
"""
if pad_x is None:
pad_x = pad
if pad_y is None:
pad_y = pad
y_min, y_max = y.min(), y.max()
x_min, x_max = x.min(), x.max()
- ax.set_ylim(y_min - pad_x * (y_max - y_min), y_max + pad_x * (y_max - y_min))
- ax.set_xlim(x_min - pad_y * (x_max - x_min), x_max + pad_y * (x_max - x_min))
+ ax.set_xlim(x_min - pad_x * (x_max - x_min), x_max + pad_x * (x_max - x_min))
+ ax.set_ylim(y_min - pad_y * (y_max - y_min), y_max + pad_y * (y_max - y_min))📝 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
| pad: The default padding factor applied to both axes. | |
| pad_x: Padding near the x-axis (vertical direction). Overrides ``pad`` when set. | |
| pad_y: Padding near the y-axis (horizontal direction). Overrides ``pad`` when set. | |
| """ | |
| if pad_x is None: | |
| pad_x = pad | |
| if pad_y is None: | |
| pad_y = pad | |
| y_min, y_max = y.min(), y.max() | |
| x_min, x_max = x.min(), x.max() | |
| ax.set_ylim(y_min - pad * (y_max - y_min), y_max + pad * (y_max - y_min)) | |
| ax.set_xlim(x_min - pad * (x_max - x_min), x_max + pad * (x_max - x_min)) | |
| ax.set_ylim(y_min - pad_x * (y_max - y_min), y_max + pad_x * (y_max - y_min)) | |
| ax.set_xlim(x_min - pad_y * (x_max - x_min), x_max + pad_y * (x_max - x_min)) | |
| pad: The default padding factor applied to both axes. | |
| pad_x: Padding applied to x-axis limits (horizontal direction). Overrides ``pad`` when set. | |
| pad_y: Padding applied to y-axis limits (vertical direction). Overrides ``pad`` when set. | |
| """ | |
| if pad_x is None: | |
| pad_x = pad | |
| if pad_y is None: | |
| pad_y = pad | |
| y_min, y_max = y.min(), y.max() | |
| x_min, x_max = x.min(), x.max() | |
| ax.set_xlim(x_min - pad_x * (x_max - x_min), x_max + pad_x * (x_max - x_min)) | |
| ax.set_ylim(y_min - pad_y * (y_max - y_min), y_max + pad_y * (y_max - y_min)) |
🤖 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 24 - 37, The pad_x and pad_y
parameter names are counterintuitive because pad_x is currently applied to
ax.set_ylim and pad_y to ax.set_xlim; update the function so pad_x controls
x-axis padding and pad_y controls y-axis padding by swapping the arguments
passed to ax.set_xlim and ax.set_ylim (or alternatively rename the parameters
and update the docstring/tests accordingly); specifically modify the code using
pad_x, pad_y, ax.set_xlim, ax.set_ylim, x, and y so that x_min/x_max use pad_x
and y_min/y_max use pad_y and adjust the docstring and any tests that depend on
the previous semantics.
Comment on lines
+137
to
+141
| try: | ||
| target_ax.set_xticklabels([t.get_text() for t in source_ax.get_xticklabels()]) | ||
| target_ax.set_yticklabels([t.get_text() for t in source_ax.get_yticklabels()]) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Avoid bare except Exception: pass — at minimum, catch a specific exception type.
Catching all exceptions and silently passing hides potential bugs. If tick label copying can fail, catch the specific exception (e.g., ValueError, IndexError) or log a warning.
🛡️ Proposed fix
try:
target_ax.set_xticklabels([t.get_text() for t in source_ax.get_xticklabels()])
target_ax.set_yticklabels([t.get_text() for t in source_ax.get_yticklabels()])
- except Exception:
- pass
+ except (ValueError, RuntimeError):
+ # Tick labels may not be settable for certain axis configurations
+ pass📝 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
| try: | |
| target_ax.set_xticklabels([t.get_text() for t in source_ax.get_xticklabels()]) | |
| target_ax.set_yticklabels([t.get_text() for t in source_ax.get_yticklabels()]) | |
| except Exception: | |
| pass | |
| try: | |
| target_ax.set_xticklabels([t.get_text() for t in source_ax.get_xticklabels()]) | |
| target_ax.set_yticklabels([t.get_text() for t in source_ax.get_yticklabels()]) | |
| except (ValueError, RuntimeError): | |
| # Tick labels may not be settable for certain axis configurations | |
| pass |
🧰 Tools
🪛 Ruff (0.15.5)
[error] 140-141: try-except-pass detected, consider logging the exception
(S110)
[warning] 140-140: Do not catch blind exception: Exception
(BLE001)
🤖 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 137 - 141, The try/except
around target_ax.set_xticklabels and set_yticklabels is swallowing all errors;
replace the bare "except Exception: pass" with catching the specific exceptions
that can occur (e.g., ValueError, IndexError, AttributeError) and surface them
via logging or warnings (e.g., call logging.getLogger(__name__).warning(...) or
warnings.warn(...)) so failures copying tick labels are recorded; update the
block around target_ax.set_xticklabels([t.get_text() for t in
source_ax.get_xticklabels()]) and target_ax.set_yticklabels([t.get_text() for t
in source_ax.get_yticklabels()]) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_plotutils.py (1)
168-172: Unused variableticksflagged by static analysis.The
ticksvariable is unpacked but not used in this test. Per Ruff RUF059, prefix with underscore.♻️ Proposed fix
def test_nice_tick_bounds_already_nice(): """When data already spans a tick-aligned range, bounds should match.""" - lo, hi, ticks = _nice_tick_bounds(0, 10) + lo, hi, _ticks = _nice_tick_bounds(0, 10) assert lo == 0.0 assert hi == 10.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_plotutils.py` around lines 168 - 172, The test test_nice_tick_bounds_already_nice unpacks a third value into ticks from _nice_tick_bounds but never uses it, triggering the RUF059 unused variable warning; update the test to unpack it as _ticks (or rename the local to _ticks) so the unused value is properly ignored while keeping the assertions on lo and hi against _nice_tick_bounds unchanged.src/lama_aesthetics/plotutils.py (1)
238-238: Avoid conditional expression with side effects.The ternary expression
... if condition else Noneis used for its side effect rather than its value. This is an anti-pattern that makes the code harder to read and reason about.♻️ Proposed fix
- target_ax.spines[spine_name].set_bounds(*source_ax.spines[spine_name].get_bounds()) if source_ax.spines[spine_name].get_bounds() else None + bounds = source_ax.spines[spine_name].get_bounds() + if bounds: + target_ax.spines[spine_name].set_bounds(*bounds)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lama_aesthetics/plotutils.py` at line 238, The line using a ternary for side effects should be replaced with a normal if statement: get the bounds once (e.g., bounds = source_ax.spines[spine_name].get_bounds()), and if bounds is truthy call target_ax.spines[spine_name].set_bounds(*bounds); reference the existing symbols spine_name, source_ax, and target_ax in plotutils.py and remove the conditional expression to improve readability and avoid side-effect usage in an expression.
🤖 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 12-21: The function _get_axis_bounds doesn't handle empty inputs
and will call numeric_arr.min()/max() causing ValueError; update
_get_axis_bounds to check for empty values (e.g., if len(arr) == 0 or arr.size
== 0) up front and return a clear empty sentinel such as (0, -1, False) (or
another agreed sentinel) so callers can detect "no data"; apply this check
before attempting the float conversion and before using max(len(arr)-1,0), and
ensure both the categorical fallback and numeric path avoid min()/max() on empty
arrays.
- Around line 270-276: The scatter call assumes sizes has at least one element
and will raise IndexError if source_coll.get_sizes() returns an empty array;
update the logic around sizes (the variable assigned from
source_coll.get_sizes()) used in the target_ax.scatter(...) call so that if
sizes is empty you pass a sensible fallback scalar size (e.g., a default scatter
size) instead of indexing sizes[0], and preserve passing the array when it is
non-empty; adjust the conditional used for s= to check for emptiness (e.g.,
sizes.size == 0 or len(sizes) == 0) and use the fallback.
---
Nitpick comments:
In `@src/lama_aesthetics/plotutils.py`:
- Line 238: The line using a ternary for side effects should be replaced with a
normal if statement: get the bounds once (e.g., bounds =
source_ax.spines[spine_name].get_bounds()), and if bounds is truthy call
target_ax.spines[spine_name].set_bounds(*bounds); reference the existing symbols
spine_name, source_ax, and target_ax in plotutils.py and remove the conditional
expression to improve readability and avoid side-effect usage in an expression.
In `@tests/test_plotutils.py`:
- Around line 168-172: The test test_nice_tick_bounds_already_nice unpacks a
third value into ticks from _nice_tick_bounds but never uses it, triggering the
RUF059 unused variable warning; update the test to unpack it as _ticks (or
rename the local to _ticks) so the unused value is properly ignored while
keeping the assertions on lo and hi against _nice_tick_bounds unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aa7936c-98e2-4390-bb17-4bc01b579b65
📒 Files selected for processing (3)
README.mdsrc/lama_aesthetics/plotutils.pytests/test_plotutils.py
Comment on lines
+12
to
+21
| def _get_axis_bounds(values): | ||
| """Return axis bounds for numeric or categorical values.""" | ||
| arr = np.asarray(values) | ||
|
|
||
| try: | ||
| numeric_arr = np.asarray(values, dtype=float) | ||
| except (TypeError, ValueError): | ||
| return 0, max(len(arr) - 1, 0), False | ||
|
|
||
| return numeric_arr.min(), numeric_arr.max(), True |
There was a problem hiding this comment.
Empty array edge case not handled.
When values is empty, numeric_arr.min() and numeric_arr.max() will raise a ValueError. Similarly, max(len(arr) - 1, 0) returns 0 for empty arrays, but the caller may not expect (0, 0, False) for empty input.
Consider adding explicit handling or documenting the expected behavior for empty inputs.
🛡️ Proposed fix
def _get_axis_bounds(values):
"""Return axis bounds for numeric or categorical values."""
arr = np.asarray(values)
+ if arr.size == 0:
+ return 0, 0, False
try:
numeric_arr = np.asarray(values, dtype=float)
except (TypeError, ValueError):
return 0, max(len(arr) - 1, 0), False
return numeric_arr.min(), numeric_arr.max(), True🤖 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 12 - 21, The function
_get_axis_bounds doesn't handle empty inputs and will call
numeric_arr.min()/max() causing ValueError; update _get_axis_bounds to check for
empty values (e.g., if len(arr) == 0 or arr.size == 0) up front and return a
clear empty sentinel such as (0, -1, False) (or another agreed sentinel) so
callers can detect "no data"; apply this check before attempting the float
conversion and before using max(len(arr)-1,0), and ensure both the categorical
fallback and numeric path avoid min()/max() on empty arrays.
Comment on lines
+270
to
+276
| sizes = source_coll.get_sizes() | ||
| target_ax.scatter( | ||
| offsets[:, 0], | ||
| offsets[:, 1], | ||
| color=facecolors if len(facecolors) > 1 else facecolors[0], | ||
| edgecolors=edgecolors if len(edgecolors) > 1 else edgecolors[0], | ||
| s=sizes if len(sizes) > 1 else sizes[0], |
There was a problem hiding this comment.
Potential IndexError if sizes array is empty.
The code assumes sizes has at least one element when using sizes[0], but get_sizes() can return an empty array for scatter plots created with default sizes that haven't been rendered yet.
🛡️ Proposed fix
facecolors = source_coll.get_facecolor()
edgecolors = source_coll.get_edgecolor()
sizes = source_coll.get_sizes()
target_ax.scatter(
offsets[:, 0],
offsets[:, 1],
- color=facecolors if len(facecolors) > 1 else facecolors[0],
- edgecolors=edgecolors if len(edgecolors) > 1 else edgecolors[0],
- s=sizes if len(sizes) > 1 else sizes[0],
+ color=facecolors if len(facecolors) > 1 else (facecolors[0] if len(facecolors) else None),
+ edgecolors=edgecolors if len(edgecolors) > 1 else (edgecolors[0] if len(edgecolors) else None),
+ s=sizes if len(sizes) > 1 else (sizes[0] if len(sizes) else None),
alpha=source_coll.get_alpha(),
label=label,
)📝 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
| sizes = source_coll.get_sizes() | |
| target_ax.scatter( | |
| offsets[:, 0], | |
| offsets[:, 1], | |
| color=facecolors if len(facecolors) > 1 else facecolors[0], | |
| edgecolors=edgecolors if len(edgecolors) > 1 else edgecolors[0], | |
| s=sizes if len(sizes) > 1 else sizes[0], | |
| sizes = source_coll.get_sizes() | |
| target_ax.scatter( | |
| offsets[:, 0], | |
| offsets[:, 1], | |
| color=facecolors if len(facecolors) > 1 else (facecolors[0] if len(facecolors) else None), | |
| edgecolors=edgecolors if len(edgecolors) > 1 else (edgecolors[0] if len(edgecolors) else None), | |
| s=sizes if len(sizes) > 1 else (sizes[0] if len(sizes) else None), | |
| alpha=source_coll.get_alpha(), | |
| label=label, | |
| ) |
🤖 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 270 - 276, The scatter call
assumes sizes has at least one element and will raise IndexError if
source_coll.get_sizes() returns an empty array; update the logic around sizes
(the variable assigned from source_coll.get_sizes()) used in the
target_ax.scatter(...) call so that if sizes is empty you pass a sensible
fallback scalar size (e.g., a default scatter size) instead of indexing
sizes[0], and preserve passing the array when it is non-empty; adjust the
conditional used for s= to check for emptiness (e.g., sizes.size == 0 or
len(sizes) == 0) and use the fallback.
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
2 participants
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 CodeRabbit
New Features
Chores
Documentation
Tests