Add Return Value to Series.plot #105
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Series as Series.plot()
participant plt as Matplotlib
Caller->>Series: Invoke plot(frame_idx, scale, **kwargs)
Series->>plt: Call gcf() to capture current figure
plt-->>Series: Return current figure (fig)
Series->>plt: Call close(fig) to close the captured figure
Series->>Caller: Return fig
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 77.44% 82.20% +4.75%
==========================================
Files 13 13
Lines 1419 1422 +3
==========================================
+ Hits 1099 1169 +70
+ Misses 320 253 -67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
sleap_roots/series.py (1)
270-277: 🛠️ Refactor suggestionUpdate the docstring to document the return value.
The docstring needs to be updated to document that the method now returns a matplotlib figure object.
"""Plot predictions on top of the image. Args: frame_idx: Frame index to visualize. scale: Relative size of the visualized image. Useful for plotting smaller images within notebooks. + + Returns: + A matplotlib.figure.Figure object containing the plot of predictions on top of the image. """
🧹 Nitpick comments (3)
sleap_roots/series.py (3)
270-270: Update the method signature to reflect the new return value.Now that the
plotmethod returns a figure object, the method signature should be updated to indicate the return type.-def plot(self, frame_idx: int, scale: float = 1.0, **kwargs): +def plot(self, frame_idx: int, scale: float = 1.0, **kwargs) -> matplotlib.figure.Figure:
314-315: Improve comment clarity.The current comment may be slightly misleading since the code is getting the figure but not displaying it.
- # Get the current figure and display it + # Get the current figure for returning to the caller fig = plt.gcf()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 315-315: sleap_roots/series.py#L315
Added line #L315 was not covered by tests
317-318: Consider a more robust approach for managing figure display.The hardcoded figure ID (1) may be brittle in different contexts. Consider a more flexible approach.
- # Close one window to prevent plot from displaying twice - plt.close(1) + # Close the current figure window to prevent display while still returning the figure object + plt.close(fig)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 318-318: sleap_roots/series.py#L318
Added line #L318 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap_roots/series.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
sleap_roots/series.py
[warning] 315-315: sleap_roots/series.py#L315
Added line #L315 was not covered by tests
[warning] 318-318: sleap_roots/series.py#L318
Added line #L318 was not covered by tests
[warning] 320-320: sleap_roots/series.py#L320
Added line #L320 was not covered by tests
🔇 Additional comments (1)
sleap_roots/series.py (1)
314-320: LGTM! The implementation satisfies the PR objective.The changes successfully implement the required feature of returning a matplotlib figure from the plot method, which aligns with the PR objective of "Add Return Value to Series.plot".
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 315-315: sleap_roots/series.py#L315
Added line #L315 was not covered by tests
[warning] 318-318: sleap_roots/series.py#L318
Added line #L318 was not covered by tests
[warning] 320-320: sleap_roots/series.py#L320
Added line #L320 was not covered by tests
a858d45 to
d5a8319
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sleap_roots/series.py (1)
324-324: Consider a more targeted approach to closing plots.The current implementation uses
plt.close("all")which closes all open plots. While this works for the current use case, a more targeted approach might be less disruptive if the function is used in a context where other plots exist.- # Close all current open plots to avoid duplication. - plt.close("all") + # Close only the current figure to avoid duplication. + plt.close(fig)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 324-324: sleap_roots/series.py#L324
Added li 8000 ne #L324 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap_roots/series.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
sleap_roots/series.py
[warning] 320-320: sleap_roots/series.py#L320
Added line #L320 was not covered by tests
[warning] 324-324: sleap_roots/series.py#L324
Added line #L324 was not covered by tests
[warning] 327-327: sleap_roots/series.py#L327
Added line #L327 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests (windows-2022, Python 3.11)
🔇 Additional comments (3)
sleap_roots/series.py (3)
270-272: Function signature update looks good.The return type annotation correctly specifies the return type as
matplotlib.figure.Figure, which aligns with the PR objectives to return a figure object instead of None.
280-281: Docstring update is appropriate.The docstring has been properly updated to document the return value, maintaining consistency between the code implementation and documentation.
319-327: Implementation correctly returns the figure object, but needs tests.The implementation correctly captures the current figure, closes all open plots to prevent duplication, and returns the figure object. This matches the PR objectives to return a matplotlib figure.
However, these new lines are not covered by tests according to the static analysis.
Consider adding a test case that verifies:
- The plot method returns a valid matplotlib figure object
- The resource cleanup (plt.close) is working as expected
#!/bin/bash # Check for existing test patterns related to plot functionality rg -A 2 -B 2 "def test.*plot" --type py🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 320-320: sleap_roots/series.py#L320
Added line #L320 was not covered by tests
[warning] 324-324: sleap_roots/series.py#L324
Added line #L324 was not covered by tests
[warning] 327-327: sleap_roots/series.py#L327
Added line #L327 was not covered by tests
plotfunction inseries.pyto return a matplotlib figure instead of returning NoneTypesleap-roots/sleap_roots/series.py
Line 270 in a858d45
Resolves #101
Summary by CodeRabbit
New Features
Tests