feat: add line plot #84
Conversation
WalkthroughThe changes introduce a new module, Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 (
|
qodo-merge-pro
bot
added
documentation
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Reviewer Guide 🔍
|
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Code Suggestions ✨
|
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. CI Failure Feedback 🧐
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
pyretailscience/plots/line.py
Outdated
| from pyretailscience.style.graph_utils import GraphStyles | ||
| from pyretailscience.style.tailwind import COLORS | ||
|
|
||
| logging.basicConfig(format="%(message)s", level=logging.INFO) |
There was a problem hiding this comment.
Avoid configuring logging in library modules
Calling logging.basicConfig() in a library module can interfere with the logging configuration of applications that import this module. It's best practice to configure logging in the main application, not in library code.
Apply this diff to remove the logging configuration from the module:
-logging.basicConfig(format="%(message)s", level=logging.INFO)This ensures that the logging configuration remains under the control of the application using the library.
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.
| logging.basicConfig(format="%(message)s", level=logging.INFO) |
pyretailscience/plots/line.py
Outdated
| title=title if title else f"{value_col.title()} Over {x_col.title()}" if x_col else f"{value_col.title()} Over Index", | ||
| x_label=x_label if x_label else (x_col.title() if x_col else "Index"), | ||
| y_label=y_label or value_col.title(), |
There was a problem hiding this comment.
Handle value_col when it's a list to prevent errors
The code assumes value_col is a string and calls value_col.title(). If value_col is a list, this will raise an AttributeError. You need to handle both string and list types appropriately.
Apply this diff to correctly handle cases where value_col is a list:
- title=title if title else f"{value_col.title()} Over {x_col.title()}" if x_col else f"{value_col.title()} Over Index",
- y_label=y_label or value_col.title(),
+ if title is None:
+ value_col_str = ', '.join(value_col) if isinstance(value_col, list) else value_col
+ x_col_str = x_col.title() if x_col else 'Index'
+ title = f"{value_col_str.title()} Over {x_col_str}"
+ if y_label is None:
+ y_label = ', '.join(value_col).title() if isinstance(value_col, list) else value_col.title()
+
+ ax = gu.standard_graph_styles(
+ ax,
+ title=title,
+ x_label=x_label if x_label else (x_col.title() if x_col else "Index"),
+ y_label=y_label,
+ )This modification ensures that when value_col is a list, it joins the column names into a string before applying .title().
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.
| title=title if title else f"{value_col.title()} Over {x_col.title()}" if x_col else f"{value_col.title()} Over Index", | |
| x_label=x_label if x_label else (x_col.title() if x_col else "Index"), | |
| y_label=y_label or value_col.title(), | |
| if title is None: | |
| value_col_str = ', '.join(value_col) if isinstance(value_col, list) else value_col | |
| x_col_str = x_col.title() if x_col else 'Index' | |
| title = f"{value_col_str.title()} Over {x_col_str}" | |
| if y_label is None: | |
| y_label = ', '.join(value_col).title() if isinstance(value_col, list) else value_col.title() | |
| ax = gu.standard_graph_styles( | |
| ax, | |
| title=title, | |
| x_label=x_label if x_label else (x_col.title() if x_col else "Index"), | |
| y_label=y_label, | |
| ) |
|
@Mrglglglglgl I forgot to mention in the ticket that you should also create the markdown doc for the API documentation. eg see this one |
…entation examples
Codecov ReportAttention: Patch coverage is
|
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
pyretailscience/style/tailwind.py (1)
348-348: Reduced color variety in the base colormap.The
color_orderlist has been reduced from a comprehensive set of Tailwind colors to only 7 colors. This change will result in a smaller color palette for the base colormap.Please verify if the new color palette meets your plotting requirements and provides sufficient visual variety for your use cases.
pyretailscience/plots/line.py (1)
270-271: Correct the return type annotation for theplotfunctionThe return type annotation currently uses
SubplotBase, which is a base class and may not be specific enough. It's more appropriate to useAxesas the return type sinceAxesis the standard type for matplotlib axes objects.Apply this diff to correct the return type:
- Returns: - SubplotBase: The matplotlib axes object. + Returns: + Axes: The matplotlib axes object.And update the function signature accordingly:
) -> SubplotBase: + ) -> Axes:
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pyretailscience/plots/line.py (1 hunks)
- pyretailscience/style/graph_utils.py (1 hunks)
- pyretailscience/style/tailwind.py (1 hunks)
Files skipped from review due to trivial changes (1)
- pyretailscience/style/graph_utils.py
Additional context used
GitHub Check: codecov/patch
pyretailscience/plots/line.py
[warning] 199-200: pyretailscience/plots/line.py#L199-L200
Added lines #L199 - L200 were not covered by tests
[warning] 202-203: pyretailscience/plots/line.py#L202-L203
Added lines #L202 - L203 were not covered by tests
[warning] 205-206: pyretailscience/plots/line.py#L205-L206
Added lines #L205 - L206 were not covered by tests
[warning] 209-209: pyretailscience/plots/line.py#L209
Added line #L209 was not covered by tests
[warning] 212-212: pyretailscience/plots/line.py#L212
Added line #L212 was not covered by tests
[warning] 223-224: pyretailscience/plots/line.py#L223-L224
Added lines #L223 - L224 were not covered by tests
[warning] 226-228: pyretailscience/plots/line.py#L226-L228
Added lines #L226 - L228 were not covered by tests
[warning] 235-236: pyretailscience/plots/line.py#L235-L236
Added lines #L235 - L236 were not covered by tests
[warning] 239-239: pyretailscience/plots/line.py#L239
Added line #L239 was not covered by tests
[warning] 273-273: pyretailscience/plots/line.py#L273
Added line #L273 was not covered by tests
[warning] 275-275: pyretailscience/plots/line.py#L275
Added line #L275 was not covered by tests
[warning] 278-278: pyretailscience/plots/line.py#L278
Added line #L278 was not covered by tests
[warning] 280-280: pyretailscience/plots/line.py#L280
Added line #L280 was not covered by tests
[warning] 282-282: pyretailscience/plots/line.py#L282
Added line #L282 was not covered by tests
[warning] 290-290: pyretailscience/plots/line.py#L290
Added line #L290 was not covered by tests
[warning] 298-298: pyretailscience/plots/line.py#L298
Added line #L298 was not covered by tests
[warning] 301-301: pyretailscience/plots/line.py#L301
Added line #L301 was not covered by tests
[warning] 304-304: pyretailscience/plots/line.py#L304
Added line #L304 was not covered by tests
[warning] 306-306: pyretailscience/plots/line.py#L306
Added line #L306 was not covered by tests
pyretailscience/plots/line.py
Outdated
| ax.legend(title=legend_title) | ||
|
|
There was a problem hiding this comment.
Correct legend title application when moving legend outside
When move_legend_outside is True and legend_title is provided, the current code may not set the legend title correctly due to the order of operations.
Adjust the code to ensure the legend title is set after moving the legend:
if move_legend_outside:
ax.legend(bbox_to_anchor=(1.05, 1))
if legend_title is not None:
- ax.legend(title=legend_title)
+ legend = ax.get_legend()
+ if legend is not None:
+ legend.set_title(legend_title)This modification retrieves the legend object and sets its title, ensuring the title appears whether the legend is inside or outside the plot.
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.
| ax.legend(title=legend_title) | |
| if move_legend_outside: | |
| ax.legend(bbox_to_anchor=(1.05, 1)) | |
| if legend_title is not None: | |
| legend = ax.get_legend() | |
| if legend is not None: | |
| legend.set_title(legend_title) |
Tools
GitHub Check: codecov/patch
[warning] 301-301: pyretailscience/plots/line.py#L301
Added line #L301 was not covered by tests
pyretailscience/plots/line.py
Outdated
| _check_datetime_column(df, x_col) | ||
|
|
There was a problem hiding this comment.
Avoid unnecessary datetime checks when x_col is not provided
Currently, _check_datetime_column is called only if x_col is not None, but if x_col is None, using the index could still pose datetime issues. Consider checking the index for datetime types when x_col is None.
Modify the code to include datetime checks on the index:
if x_col is not None:
_check_datetime_column(df, x_col)
+else:
+ if isinstance(df.index, pd.DatetimeIndex):
+ warnings.warn(
+ "The DataFrame index is datetime-like. Consider using the 'time_plot' module for time-based plots.",
+ UserWarning,
+ stacklevel=2,
+ )This ensures that users are warned if the index is datetime-like, prompting them to use the appropriate plotting module.
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.
| _check_datetime_column(df, x_col) | |
| if x_col is not None: | |
| _check_datetime_column(df, x_col) | |
| else: | |
| if isinstance(df.index, pd.DatetimeIndex): | |
| warnings.warn( | |
| "The DataFrame index is datetime-like. Consider using the 'time_plot' module for time-based plots.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) |
Tools
GitHub Check: codecov/patch
[warning] 273-273: pyretailscience/plots/line.py#L273
Added line #L273 was not covered by tests
pyretailscience/plots/line.py
Outdated
| colors: ListedColormap = get_base_cmap() | ||
|
|
||
| if group_col is not None: | ||
| pivot_df = df.pivot(index=x_col if x_col is not None else None, columns=group_col, values=value_col) | ||
| else: | ||
| pivot_df = df.set_index(x_col if x_col is not None else df.index)[value_col] | ||
|
|
||
| ax = pivot_df.plot( | ||
| ax=ax, | ||
| linewidth=3, | ||
| color=colors.colors[: len(pivot_df.columns) if group_col else 1], | ||
| legend=(group_col is not None), |
There was a problem hiding this comment.
Ensure color assignment scales with the number of groups
The current method of assigning colors slices the colors.colors list but does not handle cases where the number of groups exceeds the number of available colors. This could result in lines sharing the same color, making the plot confusing.
Consider using a colormap or an iterator to cycle through colors for an arbitrary number of groups. Apply this diff to improve color assignment:
-import warnings
from typing import TYPE_CHECKING
import pandas as pd
from matplotlib.axes import Axes, SubplotBase
-import pyretailscience.style.graph_utils as gu
-from pyretailscience.style.tailwind import get_base_cmap
+import matplotlib.pyplot as plt
+from itertools import cycle
if TYPE_CHECKING:
from matplotlib.colors import ListedColormap
# ... existing code ...
if group_col is not None:
- colors: ListedColormap = get_base_cmap()
+ color_cycle = cycle(plt.cm.tab20.colors)
+ colors = [next(color_cycle) for _ in range(len(pivot_df.columns))]
else:
- colors: ListedColormap = get_base_cmap()
+ colors = ['blue'] # Default color for single line
ax = pivot_df.plot(
ax=ax,
linewidth=3,
- color=colors.colors[: len(pivot_df.columns) if group_col else 1],
+ color=colors,
legend=(group_col is not None),
**kwargs,
)This change leverages matplotlib's tab20 colormap and cycles through colors to accommodate any number of groups.
Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 275-275: pyretailscience/plots/line.py#L275
Added line #L275 was not covered by tests
[warning] 278-278: pyretailscience/plots/line.py#L278
Added line #L278 was not covered by tests
[warning] 280-280: pyretailscience/plots/line.py#L280
Added line #L280 was not covered by tests
[warning] 282-282: pyretailscience/plots/line.py#L282
Added line #L282 was not covered by tests
pyretailscience/plots/line.py
Outdated
|
|
||
| if TYPE_CHECKING: | ||
| from matplotlib.colors import ListedColormap | ||
|
|
||
|
|
||
| def _check_datetime_column(df: pd.DataFrame, x_col: str) -> None: | ||
| """Checks if the x_col is a datetime or convertible to datetime. | ||
|
|
||
| Issues a warning if the column is datetime-like, recommending | ||
| the use of a time-based plot. | ||
|
|
||
| Args: | ||
| df (pd.DataFrame): The dataframe containing the column to check. | ||
| x_col (str): The column to check for datetime-like values. | ||
| """ | ||
| if x_col not in df.columns: | ||
| msg = f"The column '{x_col}' is not present in the dataframe." | ||
| raise KeyError(msg) | ||
|
|
||
| try: | ||
| pd.to_datetime(df[x_col], errors="raise") | ||
| warnings.warn( | ||
| f"The column '{x_col}' can be converted to datetime. Consider using the 'time_plot' module for time-based " | ||
| f"plots.", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) | ||
|
|
||
| except (ValueError, TypeError): | ||
| return | ||
|
|
||
|
|
||
| def plot( | ||
| df: pd.DataFrame, | ||
| value_col: str | list[str], | ||
| x_label: str, | ||
| y_label: str, | ||
| title: str, | ||
| x_col: str | None = None, | ||
| group_col: str | None = None, | ||
| legend_title: str | None = None, | ||
| ax: Axes | None = None, | ||
| source_text: str | None = None, | ||
| move_legend_outside: bool = False, | ||
| **kwargs: dict[str, any], | ||
| ) -> SubplotBase: | ||
| """Plots the `value_col` over the specified `x_col` or index, creating a separate line for each unique value in `group_col`. | ||
|
|
||
| Args: | ||
| df (pd.DataFrame): The dataframe to plot. | ||
| value_col (str or list of str): The column(s) to plot. | ||
| x_label (str): The x-axis label. | ||
| y_label (str): The y-axis label. | ||
| title (str): The title of the plot. | ||
| x_col (str, optional): The column to be used as the x-axis. If None, the index is used. | ||
| group_col (str, optional): The column used to define different lines. | ||
| legend_title (str, optional): The title of the legend. | ||
| ax (Axes, optional): Matplotlib axes object to plot on. | ||
| source_text (str, optional): The source text to add to the plot. | ||
| move_legend_outside (bool, optional): Move the legend outside the plot. | ||
| **kwargs: Additional keyword arguments for Pandas' `plot` function. | ||
|
|
||
| Returns: | ||
| SubplotBase: The matplotlib axes object. | ||
| """ | ||
| if x_col is not None: | ||
| _check_datetime_column(df, x_col) | ||
|
|
||
| colors: ListedColormap = get_base_cmap() | ||
|
|
||
| if group_col is not None: | ||
| pivot_df = df.pivot(index=x_col if x_col is not None else None, columns=group_col, values=value_col) | ||
| else: | ||
| pivot_df = df.set_index(x_col if x_col is not None else df.index)[value_col] | ||
|
|
||
| ax = pivot_df.plot( | ||
| ax=ax, | ||
| linewidth=3, | ||
| color=colors.colors[: len(pivot_df.columns) if group_col else 1], | ||
| legend=(group_col is not None), | ||
| **kwargs, | ||
| ) | ||
|
|
||
| ax = gu.standard_graph_styles( | ||
| ax, | ||
| title=title, | ||
| x_label=x_label, | ||
| y_label=y_label, | ||
| ) | ||
|
|
||
| if move_legend_outside: | ||
| ax.legend(bbox_to_anchor=(1.05, 1)) | ||
|
|
||
| if legend_title is not None: | ||
| ax.legend(title=legend_title) | ||
|
|
||
| if source_text is not None: | ||
| gu.add_source_text(ax=ax, source_text=source_text) | ||
|
|
||
| return gu.standard_tick_styles(ax) |
There was a problem hiding this comment.
Tip
Codebase Verification
Unit tests needed for functions in line.py
Our analysis confirms that there are currently no unit tests covering the functions in line.py, including _check_datetime_column and plot. To improve code quality and prevent potential regressions, we recommend:
- Creating a new test file
tests/test_line.pyspecifically for testing the functions inline.py. - Writing unit tests for both
_check_datetime_columnandplotfunctions, covering various scenarios and edge cases. - Ensuring that these new tests are integrated into the existing test suite and run as part of the regular testing process.
Adding these tests will significantly improve the overall test coverage and help maintain the reliability of the line.py module.
Analysis chain
Consider adding unit tests for the new functions to improve test coverage
The static analysis hints indicate that several lines of newly added code are not covered by tests, particularly in the _check_datetime_column and plot functions. Adding unit tests for these functions will help ensure their correctness and prevent future regressions.
To identify areas lacking test coverage, you can run the following script:
This script assumes you have a tests/ directory containing your test suite and that you're using the coverage package to measure test coverage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Identify functions in 'line.py' not covered by tests.
# Test: Run coverage analysis. Expect: All functions are covered by tests.
coverage run -m pytest tests/
coverage report -m pyretailscience/plots/line.py
Length of output: 234
Script:
#!/bin/bash
# Search for test files related to line.py
echo "Searching for test files related to line.py:"
fd test_line.py tests/
# Look for test functions related to _check_datetime_column and plot
echo "Searching for test functions related to _check_datetime_column and plot:"
rg -n "def test_.*(_check_datetime_column|plot)" tests/
# Examine the structure of the test directory
echo "Structure of the test directory:"
tree tests/ -L 2
Length of output: 1222
Script:
#!/bin/bash
# Check for any imports or uses of line.py in test files
echo "Searching for imports or uses of line.py in test files:"
rg -n "from pyretailscience.plots.line import|import pyretailscience.plots.line" tests/
# Examine contents of test_standard_graphs.py for any potential indirect tests
echo "Examining test_standard_graphs.py for potential indirect tests:"
rg -n "line\.plot|line\._check_datetime_column" tests/test_standard_graphs.py
Length of output: 427
Tools
GitHub Check: codecov/patch
[warning] 199-200: pyretailscience/plots/line.py#L199-L200
Added lines #L199 - L200 were not covered by tests
[warning] 202-203: pyretailscience/plots/line.py#L202-L203
Added lines #L202 - L203 were not covered by tests
[warning] 205-206: pyretailscience/plots/line.py#L205-L206
Added lines #L205 - L206 were not covered by tests
[warning] 209-209: pyretailscience/plots/line.py#L209
Added line #L209 was not covered by tests
[warning] 212-212: pyretailscience/plots/line.py#L212
Added line #L212 was not covered by tests
[warning] 223-224: pyretailscience/plots/line.py#L223-L224
Added lines #L223 - L224 were not covered by tests
[warning] 226-228: pyretailscience/plots/line.py#L226-L228
Added lines #L226 - L228 were not covered by tests
[warning] 235-236: pyretailscience/plots/line.py#L235-L236
Added lines #L235 - L236 were not covered by tests
[warning] 239-239: pyretailscience/plots/line.py#L239
Added line #L239 was not covered by tests
[warning] 273-273: pyretailscience/plots/line.py#L273
Added line #L273 was not covered by tests
[warning] 275-275: pyretailscience/plots/line.py#L275
Added line #L275 was not covered by tests
[warning] 278-278: pyretailscience/plots/line.py#L278
Added line #L278 was not covered by tests
[warning] 280-280: pyretailscience/plots/line.py#L280
Added line #L280 was not covered by tests
[warning] 282-282: pyretailscience/plots/line.py#L282
Added line #L282 was not covered by tests
[warning] 290-290: pyretailscience/plots/line.py#L290
Added line #L290 was not covered by tests
[warning] 298-298: pyretailscience/plots/line.py#L298
Added line #L298 was not covered by tests
[warning] 301-301: pyretailscience/plots/line.py#L301
Added line #L301 was not covered by tests
[warning] 304-304: pyretailscience/plots/line.py#L304
Added line #L304 was not covered by tests
[warning] 306-306: pyretailscience/plots/line.py#L306
Added line #L306 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
tests/plots/test_line.py (3)
15-15: Remove parentheses from the@pytest.fixturedecorator.The fixture looks good for testing the line plot functionality. However, please remove the parentheses from the
@pytest.fixturedecorator to follow the best practices.Apply this diff:
-@pytest.fixture() +@pytest.fixture def sample_dataframe():Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Remove parentheses from the@pytest.fixturedecorator.The fixture looks good for mocking the
get_base_cmapfunction. However, please remove the parentheses from the@pytest.fixturedecorator to follow the best practices.Apply this diff:
-@pytest.fixture() +@pytest.fixture def _mock_get_base_cmap(mocker):Tools
Ruff
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Remove parentheses from the@pytest.fixturedecorator.The fixture looks good for mocking the
graph_utilsfunctions. However, please remove the parentheses from the@pytest.fixturedecorator to follow the best practices.Apply this diff:
-@pytest.fixture() +@pytest.fixture def _mock_gu_functions(mocker):Tools
Ruff
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
docs/assets/images/analysis_modules/line_plot.svgis excluded by!**/*.svgpoetry.lockis excluded by!**/*.lock
Files selected for processing (6)
- docs/analysis_modules.md (1 hunks)
- docs/api/plots/line.md (1 hunks)
- mkdocs.yml (1 hunks)
- pyproject.toml (1 hunks)
- pyretailscience/plots/line.py (1 hunks)
- tests/plots/test_line.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyretailscience/plots/line.py
Additional context used
Markdownlint
docs/analysis_modules.md
53-53: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
24-24: Expected: 120; Actual: 129
Line length(MD013, line-length)
Ruff
tests/plots/test_line.py
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Additional comments not posted (9)
mkdocs.yml (1)
26-26: LGTM!The addition of the "Line Plot" entry to the navigation menu aligns with the PR objective of introducing a new line plot module and providing documentation for it. The change is straightforward and does not introduce any apparent issues in the MkDocs configuration file.
pyproject.toml (1)
29-29: LGTM!The addition of
pytest-mockas a development dependency is a good choice to enhance the project's testing capabilities. The version constraint is appropriate and consistent with the existing dependencies.tests/plots/test_line.py (5)
40-59: LGTM!The test function looks comprehensive and covers the important aspects of plotting with a group column. The assertions check for the correct return type and the number of lines plotted.
61-79: LGTM!The test function looks comprehensive and covers the important aspects of plotting without a group column. The assertions check for the correct return type and the number of lines plotted.
81-103: LGTM!The test function looks comprehensive and covers the important aspect of warning when using a datetime x_col. The mocking of
warnings.warnand the assertion check if the warning is raised with the expected message.
105-130: LGTM!The test function looks comprehensive and covers the important aspect of moving the legend outside the plot. The assertions check if the legend exists and its
bbox_to_anchoris set to the expected coordinates.
132-149: LGTM!The test function looks comprehensive and covers the important aspect of adding the source text to the plot. The assertion checks if the
gu.add_source_textfunction was called with the expected arguments.docs/analysis_modules.md (2)
10-52: LGTM!The new "Line Plot" section provides a clear and concise description of line plots, their applications, and includes a helpful note about the scope of the module. The example code snippet is well-structured and demonstrates the usage effectively.
Tools
Markdownlint
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
24-24: Expected: 120; Actual: 129
Line length(MD013, line-length)
22-24: The long lines flagged by Markdownlint are in the description and can be left as-is for better flow.Tools
Markdownlint
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
24-24: Expected: 120; Actual: 129
Line length(MD013, line-length)
docs/analysis_modules.md
Outdated
| ) | ||
| ``` | ||
|
|
||
|
|
There was a problem hiding this comment.
Fix the consecutive blank lines.
Markdownlint has flagged multiple consecutive blank lines at line 53. Please reduce them to a single blank line to improve readability.
Apply this diff to fix the consecutive blank lines:
-
-
### Waterfall PlotCommittable 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.
| ### Waterfall Plot |
Tools
Markdownlint
53-53: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
There was a problem hiding this comment.
@Mrglglglglgl can you add the markdown linter to your setup https://plugins.jetbrains.com/plugin/20851-markdownlint. There is a .markdownlint.json file in the root of the repo with the settings, just fyi.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@Mrglglglglgl It seems that for whatever reason your matplotlib isn't exporting the background. Can you try explicitly setting it for the outputs please. Thinking about it now it could mess up some of the documentation images. Apparently this might be a pycharm related thing? https://stackoverflow.com/a/55436299
docs/analysis_modules.md
Outdated
|
|
||
| { align=right loading=lazy width="50%"} | ||
|
|
||
| Line plots are particularly good for visualizing sequences that resemble time-based data, such as: |
There was a problem hiding this comment.
| Line plots are particularly good for visualizing sequences that resemble time-based data, such as: | |
| I'd change this line to "Line plots are particularly good for visualizing sequences that are ordered or sequential, but not necessarily categorical, such as: |
docs/analysis_modules.md
Outdated
|
|
||
| They are often used to compare trends across categories, show the impact of events on performance, and visualize changes over time-like sequences. | ||
|
|
||
| Note: This module is not intended for actual datetime values. For time-based plots using dates, refer to the **timeline** module. |
There was a problem hiding this comment.
| Note: This module is not intended for actual datetime values. For time-based plots using dates, refer to the **timeline** module. | |
| Note: While this module can handle datetime values on the x-axis, the **time** plot module has additional features that make working with datetimes easier, such as easily resampling the data to alternate time frames. |
pyretailscience/plots/line.py
Outdated
| It focuses on visualizing sequences that resemble time-based data, such as "days since an event" or "months since a | ||
| competitor opened." However, it does not explicitly handle datetime values. For actual time-based plots using | ||
| datetime objects, please refer to the **`timeline`** module. |
There was a problem hiding this comment.
| It focuses on visualizing sequences that resemble time-based data, such as "days since an event" or "months since a | |
| competitor opened." However, it does not explicitly handle datetime values. For actual time-based plots using | |
| datetime objects, please refer to the **`timeline`** module. | |
| It focuses on visualizing sequences that are ordered or sequential but not necessarily categorical, such as "days since an event" or "months since a competitor opened." However, while this module can handle datetime values on the x-axis, the **timeline** module has additional features that make working with datetimes easier, such as easily resampling the data to alternate time frames. |
pyretailscience/plots/line.py
Outdated
| if group_col is not None: | ||
| pivot_df = df.pivot(index=x_col if x_col is not None else None, columns=group_col, values=value_col) | ||
| else: | ||
| pivot_df = df.set_index(x_col if x_col is not None else df.index)[value_col] |
There was a problem hiding this comment.
I'd simplify this to remove the double negative. ie not None
| if group_col is not None: | |
| pivot_df = df.pivot(index=x_col if x_col is not None else None, columns=group_col, values=value_col) | |
| else: | |
| pivot_df = df.set_index(x_col if x_col is not None else df.index)[value_col] | |
| if group_col is None: | |
| pivot_df = df.set_index(x_col if x_col is not None else df.index)[value_col] | |
| else: | |
| pivot_df = df.pivot(index=x_col if x_col is not None else None, columns=group_col, values=value_col) |
pyretailscience/plots/line.py
Outdated
| if move_legend_outside: | ||
| ax.legend(bbox_to_anchor=(1.05, 1)) | ||
|
|
||
| if legend_title is not None: | ||
| ax.legend(title=legend_title) |
There was a problem hiding this comment.
@Mrglglglglgl The coderabbit suggestion below is correct. What do you think about the below code?
| if move_legend_outside: | |
| ax.legend(bbox_to_anchor=(1.05, 1)) | |
| if legend_title is not None: | |
| ax.legend(title=legend_title) | |
| legend = ax.get_legend() | |
| if move_legend_outside: | |
| legend.set_bbox_to_anchor((1.05, 1)) | |
| if legend_title is not None: | |
| legend.set_title(title=legend_title) |
pyretailscience/plots/line.py
Outdated
| x_label (str): The x-axis label. | ||
| y_label (str): The y-axis label. | ||
| title (str): The title of the plot. |
There was a problem hiding this comment.
| x_label (str): The x-axis label. | |
| y_label (str): The y-axis label. | |
| title (str): The title of the plot. | |
| x_label (str, optional): The x-axis label. | |
| y_label (str, optional): The y-axis label. | |
| title (str, optional): The title of the plot. |
pyretailscience/plots/line.py
Outdated
| ax = gu.standard_graph_styles( | ||
| ax, | ||
| title=title, | ||
| x_label=x_label, | ||
| y_label=y_label, | ||
| ) |
There was a problem hiding this comment.
Since this is now just passing parameters
| ax = gu.standard_graph_styles( | |
| ax, | |
| title=title, | |
| x_label=x_label, | |
| y_label=y_label, | |
| ) | |
| ax = gu.standard_graph_styles(ax, title=title, x_label=x_label, y_label=y_label) |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
pyretailscience/plots/line.py (1)
9-11: Minor inconsistency in module usage warningThe docstring mentions that a warning will be triggered if a datetime column is passed as
x_col, suggesting the use of the "timeline" module. However, in the actual implementation, the warning message refers to the "time" module. Consider updating either the docstring or the implementation to ensure consistency.Apply this change to the docstring:
-**`timeline`** module. +**`time`** module.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
docs/assets/images/analysis_modules/plots/line_plot.svgis excluded by!**/*.svg
Files selected for processing (3)
- docs/analysis_modules.md (1 hunks)
- pyretailscience/plots/line.py (1 hunks)
- pyretailscience/style/graph_utils.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyretailscience/style/graph_utils.py
Additional context used
Markdownlint
docs/analysis_modules.md
16-16: Expected: 120; Actual: 132
Line length(MD013, line-length)
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
24-24: Expected: 120; Actual: 220
Line length(MD013, line-length)
GitHub Check: codecov/patch
pyretailscience/plots/line.py
[warning] 105-105: pyretailscience/plots/line.py#L105
Added line #L105 was not covered by tests
[warning] 107-107: pyretailscience/plots/line.py#L107
Added line #L107 was not covered by tests
Additional comments not posted (2)
pyretailscience/plots/line.py (1)
45-112: Improve test coverage for theplotfunctionThe static analysis hints indicate that some parts of the
plotfunction are not covered by tests, particularly the legend handling code. To ensure the reliability and maintainability of this module, it's important to have comprehensive test coverage.Consider adding the following test cases to improve coverage:
- Test with
move_legend_outside=True- Test with
legend_titleprovided- Test with both
move_legend_outside=Trueandlegend_titleprovided- Test with empty DataFrame
- Test with invalid column names
- Test with datetime index when
x_colis NoneHere's a shell script to verify the current test coverage:
This script will help identify specific lines that need additional test coverage.
Tools
GitHub Check: codecov/patch
[warning] 105-105: pyretailscience/plots/line.py#L105
Added line #L105 was not covered by tests
[warning] 107-107: pyretailscience/plots/line.py#L107
Added line #L107 was not covered by testsdocs/analysis_modules.md (1)
10-52: LGTM! The new Line Plot section is informative and well-structured.The content provides a clear overview of line plots, their applications, and includes a helpful example. However, there are a few minor formatting issues to address:
Some lines exceed the recommended length of 120 characters. Consider breaking these lines for better readability:
- Line 16
- Line 22
- Line 24
As per a previous suggestion, consider modifying line 16 to:
"Line plots are particularly good for visualizing sequences that are ordered or sequential, but not necessarily categorical, such as:"To address the line length issues, you can apply the following changes:
-Line plots are particularly good for visualizing sequences that are ordered or sequential, but not necessarily categorical, such as: +Line plots are particularly good for visualizing sequences that are ordered or sequential, +but not necessarily categorical, such as: -They are often used to compare trends across categories, show the impact of events on performance, and visualize changes over time-like sequences. +They are often used to compare trends across categories, show the impact of events on performance, +and visualize changes over time-like sequences. -Note: While this module can handle datetime values on the x-axis, the **timeline** plot module has additional features that make working with datetimes easier, such as easily resampling the data to alternate time frames. +Note: While this module can handle datetime values on the x-axis, the **timeline** plot module +has additional features that make working with datetimes easier, such as easily resampling +the data to alternate time frames.Regarding the note about the timeline plot module, let's verify if this information is accurate:
This script will help us confirm the existence of the timeline plot module and its resampling functionality.
Tools
Markdownlint
16-16: Expected: 120; Actual: 132
Line length(MD013, line-length)
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
24-24: Expected: 120; Actual: 220
Line length(MD013, line-length)
pyretailscience/plots/line.py
Outdated
| if move_legend_outside: | ||
| ax.legend(bbox_to_anchor=(1.05, 1)) | ||
|
|
||
| if legend_title is not None: | ||
| legend = ax.get_legend() | ||
| if legend is not None: | ||
| legend.set_title(legend_title) |
There was a problem hiding this comment.
Simplify legend handling
The current implementation handles the legend in two separate blocks. This can be simplified by retrieving the legend object once and then applying all modifications to it.
Consider refactoring the legend handling as follows:
-if move_legend_outside:
- ax.legend(bbox_to_anchor=(1.05, 1))
-
-if legend_title is not None:
- legend = ax.get_legend()
- if legend is not None:
- legend.set_title(legend_title)
+legend = ax.get_legend()
+if legend is not None:
+ if move_legend_outside:
+ legend.set_bbox_to_anchor((1.05, 1))
+ if legend_title is not None:
+ legend.set_title(legend_title)This change consolidates the legend modifications, making the code more efficient and easier to read.
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.
| if move_legend_outside: | |
| ax.legend(bbox_to_anchor=(1.05, 1)) | |
| if legend_title is not None: | |
| legend = ax.get_legend() | |
| if legend is not None: | |
| legend.set_title(legend_title) | |
| legend = ax.get_legend() | |
| if legend is not None: | |
| if move_legend_outside: | |
| legend.set_bbox_to_anchor((1.05, 1)) | |
| if legend_title is not None: | |
| legend.set_title(legend_title) |
Tools
GitHub Check: codecov/patch
[warning] 105-105: pyretailscience/plots/line.py#L105
Added line #L105 was not covered by tests
[warning] 107-107: pyretailscience/plots/line.py#L107
Added line #L107 was not covered by tests
pyretailscience/plots/line.py
Outdated
| if x_col is not None and pd.api.types.is_datetime64_any_dtype(df[x_col]): | ||
| warnings.warn( | ||
| f"The column '{x_col}' is datetime-like. Consider using the 'timeline' module for time-based plots.", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Extend datetime check to include DataFrame index
The current implementation only checks for datetime-like columns when x_col is provided. However, if x_col is None, the function uses the DataFrame's index, which could also be datetime-like. Consider extending the check to include the index when x_col is None.
Apply this change to include a check for the index:
if x_col is not None and pd.api.types.is_datetime64_any_dtype(df[x_col]):
warnings.warn(
f"The column '{x_col}' is datetime-like. Consider using the 'time' module for time-based plots.",
UserWarning,
stacklevel=2,
)
+elif x_col is None and pd.api.types.is_datetime64_any_dtype(df.index):
+ warnings.warn(
+ "The DataFrame index is datetime-like. Consider using the 'time' module for time-based plots.",
+ UserWarning,
+ stacklevel=2,
+ )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.
| if x_col is not None and pd.api.types.is_datetime64_any_dtype(df[x_col]): | |
| warnings.warn( | |
| f"The column '{x_col}' is datetime-like. Consider using the 'timeline' module for time-based plots.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) | |
| if x_col is not None and pd.api.types.is_datetime64_any_dtype(df[x_col]): | |
| warnings.warn( | |
| f"The column '{x_col}' is datetime-like. Consider using the 'timeline' module for time-based plots.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) | |
| elif x_col is None and pd.api.types.is_datetime64_any_dtype(df.index): | |
| warnings.warn( | |
| "The DataFrame index is datetime-like. Consider using the 'time' module for time-based plots.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) |
There was a problem hiding this comment.
@mvanwyk What do you think of this suggestion? It also suggested some more checks e.g. that the value_col for example is present in the dataframe, otherwise raise an error, and so on about other arguments that are about columns expected to be present in the dataframe
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
More specifically:
Add the following validation checks at the beginning of the plot function:
if df.empty:
raise ValueError("The input DataFrame is empty.")
if x_col is not None and x_col not in df.columns:
raise ValueError(f"Column '{x_col}' not found in the DataFrame.")
if isinstance(value_col, str):
value_col = [value_col]
for col in value_col:
if col not in df.columns:
raise ValueError(f"Column '{col}' not found in the DataFrame.")
if group_col is not None and group_col not in df.columns:
raise ValueError(f"Column '{group_col}' not found in the DataFrame.")
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tests/plots/test_line.py (1)
15-15: Minor: Consider removing parentheses from@pytest.fixture()For consistency with pytest best practices, consider removing the parentheses from the
@pytest.fixture()decorators. This is a minor stylistic change suggested by the Ruff static analysis tool.@pytest.fixture def sample_dataframe(): ... @pytest.fixture def _mock_get_base_cmap(mocker): ... @pytest.fixture def _mock_gu_functions(mocker): ...Also applies to: 26-26, 33-33
Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- pyretailscience/plots/line.py (1 hunks)
- tests/plots/test_line.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyretailscience/plots/line.py
Additional context used
Ruff
tests/plots/test_line.py
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Additional comments not posted (7)
tests/plots/test_line.py (7)
1-12: LGTM: Imports and module docstring are appropriate.The module docstring and imports are well-organized and include all necessary dependencies for the test file.
15-38: LGTM: Fixtures are well-designed and serve their purposes.The fixtures
sample_dataframe,_mock_get_base_cmap, and_mock_gu_functionsare well-implemented and provide the necessary setup for the tests. They create sample data and mock the required functions effectively.Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
40-79: LGTM: Tests for plot function with and without group column are well-implemented.The
test_plot_with_group_colandtest_plot_without_group_colfunctions effectively test theline.plotfunction for different scenarios. They use appropriate fixtures, check for the correct number of plotted lines, and verify the return type. The tests are well-structured and cover important use cases.
81-102: LGTM: Test for datetime warning is well-implemented.The
test_plot_warns_if_xcol_is_datetimefunction effectively tests the warning behavior of theline.plotfunction when a datetime-like column is used for the x-axis. It uses appropriate mocking techniques and verifies the correct warning message and stack level.
105-149: LGTM: Tests for legend placement and source text addition are well-implemented.The
test_plot_moves_legend_outsideandtest_plot_adds_source_textfunctions effectively test important features of theline.plotfunction. They verify the correct placement of the legend when moved outside and the addition of source text to the plot. The tests use appropriate assertions and mocking techniques.
1-1: Note on test organization: Current structure is appropriate.Regarding the past comment about using classes to group tests, the current structure with individual test functions is clear, follows pytest conventions, and is appropriate for the current scope of the module. While using classes can be beneficial for larger modules with more complex setups, the current organization is suitable for these tests. If the module grows significantly in the future, we can revisit this decision.
1-149: Overall: Excellent test coverage and implementation.This test file provides comprehensive coverage for the
line.plotfunction, including tests for various scenarios such as plotting with and without group columns, handling datetime x-axis, legend placement, and source text addition. The tests are well-structured, use appropriate pytest fixtures and mocking techniques, and include relevant assertions.Minor suggestions for improvement:
- Consider removing parentheses from
@pytest.fixture()decorators for consistency with pytest best practices.Great job on implementing thorough and well-organized tests for the line plotting functionality!
Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
tests/plots/test_line.py (1)
15-15: Minor style improvement: Remove parentheses from pytest.fixture decoratorsTo improve code style and consistency, consider removing the parentheses from the
@pytest.fixturedecorators. This change is suggested by the Ruff static analysis tool and would make the code slightly more concise.Apply this change to the following lines:
-@pytest.fixture() +@pytest.fixtureThis change should be applied to lines 15, 26, and 33.
Also applies to: 26-26, 33-33
Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
pyretailscience/style/graph_utils.py (1)
70-71: Great enhancements to thestandard_graph_stylesfunction!The new parameters
legend_titleandmove_legend_outsideadd valuable customization options for the legend. The implementation is clean and follows good practices. Setting the face color to white (line 90) is a nice touch for improving visual clarity.Consider adding a check for an empty string in the
legend_titlecondition:if legend_title and legend_title.strip(): legend.set_title(legend_title)This would prevent setting an empty legend title, which might be unintended.
Also applies to: 84-85, 90-90, 119-125
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pyretailscience/plots/line.py (1 hunks)
- pyretailscience/style/graph_utils.py (4 hunks)
- tests/plots/test_line.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyretailscience/plots/line.py
Additional context used
Ruff
tests/plots/test_line.py
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Additional comments not posted (14)
tests/plots/test_line.py (12)
1-13: LGTM: Imports and overall structureThe imports and overall structure of the test file are well-organized and follow good practices for pytest. The necessary modules are imported, and the custom modules are appropriately referenced.
15-23: LGTM: Well-designed sample_dataframe fixtureThe
sample_dataframefixture provides a well-structured sample dataset for testing. It includes a date range, numeric values, and a grouping column, which is suitable for testing various scenarios in line plots.Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-30: LGTM: Effective mocking of colormapThe
_mock_get_base_cmapfixture effectively mocks theget_base_cmapfunction to return a custom colormap. This approach isolates the tests from external dependencies and provides a predictable color scheme for testing.Tools
Ruff
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-37: LGTM: Comprehensive mocking of graph utility functionsThe
_mock_gu_functionsfixture effectively mocks multiple graph utility functions. This approach allows the tests to focus on the core plotting functionality without external dependencies. The use ofside_effectto return the inputaxis a good practice for maintaining the expected behavior of these utility functions.Tools
Ruff
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
40-59: LGTM: Comprehensive test for grouped data plottingThe
test_plot_with_group_colfunction effectively tests the plotting functionality with grouped data. It verifies both the return type and the correct number of lines plotted, ensuring that the grouping functionality works as expected.
61-78: LGTM: Effective test for non-grouped data plottingThe
test_plot_without_group_colfunction properly tests the plotting functionality without a grouping column. It verifies the correct return type and ensures that only one line is plotted, covering an important use case for the plotting function.
81-102: LGTM: Well-implemented warning test for datetime x-axisThe
test_plot_warns_if_xcol_is_datetimefunction effectively tests the warning mechanism for datetime-like x-axis columns. It properly mocks thewarnings.warnfunction and verifies that the correct warning message is issued, helping guide users to the appropriate module for time-based plots.
105-131: LGTM: Thorough test for legend positioning outside the plotThe
test_plot_moves_legend_outsidefunction effectively tests the legend positioning functionality. It verifies that thestandard_graph_stylesfunction is called with the correct parameters, includingmove_legend_outside=True. This ensures that the legend is properly positioned outside the plot when specified.
134-160: LGTM: Comprehensive test for legend positioning inside the plotThe
test_plot_moves_legend_insidefunction effectively complements the previous test by verifying the legend positioning inside the plot. It ensures that thestandard_graph_stylesfunction is called withmove_legend_outside=False, maintaining consistency in testing both legend positioning scenarios.
163-180: LGTM: Effective test for adding source text to the plotThe
test_plot_adds_source_textfunction properly tests the addition of source text to the plot. It verifies that theadd_source_textfunction is called with the correct parameters, ensuring that the source text functionality works as expected.
1-180: Addressing past review comments
- Regarding the use of mocking: The current implementation effectively uses mocking to isolate tests from external dependencies, which is a good practice in unit testing.
- Regarding the use of classes for grouping tests: The current structure using individual test functions is clear and maintainable. While using classes is an option, the current approach is equally valid and aligns well with pytest conventions.
Both past comments have been adequately addressed in the current implementation.
Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
1-180: Overall: Excellent test coverage and implementationThis test file for the
lineplotting module is well-structured, comprehensive, and follows good testing practices. It covers various scenarios including grouped and non-grouped data, legend positioning, and source text addition. The use of fixtures and mocking is appropriate and enhances test isolation. Only minor style improvements have been suggested, and the overall quality of the tests is high.Great job on implementing these tests!
Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
pyretailscience/style/graph_utils.py (2)
Line range hint
147-155: Docstring improvement fornot_nonefunctionThe updated docstring corrects a typo and provides clearer documentation for the function. Good attention to detail!
Line range hint
1-238: Overall improvements to graph_utils.pyThe changes to this file enhance the functionality of the
standard_graph_stylesfunction by adding legend customization options and improving graph visibility. The docstring update for thenot_nonefunction also contributes to better code documentation.These improvements align well with the file's purpose and maintain consistency with the existing code style. Great work on enhancing the graph styling capabilities!
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. CI Failure Feedback 🧐
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
tests/plots/test_line.py (4)
15-23: LGTM: Well-structured sample data fixture.The
sample_dataframefixture provides a good variety of data types (datetime, numeric, and categorical) which aligns well with the expected input for theline.plotfunction. This will enable comprehensive testing of various scenarios.Consider using
@pytest.fixtureinstead of@pytest.fixture()for consistency with pytest best practices.Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-30: LGTM: Effective mocking of colormap.The
_mock_get_base_cmapfixture effectively mocks theget_base_cmapfunction, providing a custom colormap. This approach isolates the tests from external dependencies and ensures a predictable set of colors for testing.Consider using
@pytest.fixtureinstead of@pytest.fixture()for consistency with pytest best practices.Tools
Ruff
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-37: LGTM: Effective mocking of graph utility functions.The
_mock_gu_functionsfixture efficiently mocks several graph utility functions, isolating the tests from the actual implementations. The use ofside_effectto return the inputaxobject allows for proper method chaining, which is a good practice.Consider using
@pytest.fixtureinstead of@pytest.fixture()for consistency with pytest best practices.Regarding the past comment about mocking these functions: Mocking is appropriate here as it allows us to focus on testing the
line.plotfunction's behavior without being affected by the implementation details of the utility functions. This approach enhances test isolation and makes the tests more robust against changes in the utility functions.Tools
Ruff
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
1-271: Overall: Excellent test coverage and structure for the line.plot function.This test suite provides comprehensive coverage for the
line.plotfunction, including:
- Plotting with and without grouped data
- Legend positioning and titling
- Source text addition
- Warnings for datetime columns and indices
The use of fixtures and mocking is appropriate and helps isolate the tests from external dependencies. The test functions are well-structured, consistent, and cover both normal usage and edge cases.
Minor suggestion: Consider using
@pytest.fixtureinstead of@pytest.fixture()for all fixtures to align with pytest best practices.Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- pyretailscience/plots/line.py (1 hunks)
- tests/plots/test_line.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyretailscience/plots/line.py
Additional context used
Ruff
tests/plots/test_line.py
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Additional comments not posted (10)
tests/plots/test_line.py (10)
1-12: LGTM: Import statements are appropriate and concise.The import statements are well-organized, importing necessary modules and functions from both standard libraries and custom modules. This setup provides a good foundation for the test suite.
40-59: LGTM: Comprehensive test for plotting with grouped data.The
test_plot_with_group_colfunction effectively tests theline.plotfunction with grouped data. It verifies both the return type (Axes) and the number of lines plotted, ensuring that the grouping functionality works as expected.
61-78: LGTM: Effective test for plotting without grouped data.The
test_plot_without_group_colfunction provides a good complement to the grouped data test. It verifies that theline.plotfunction correctly handles non-grouped data, asserting both the return type and the number of lines plotted. This maintains consistency with the previous test while covering a different scenario.
81-102: LGTM: Good test for datetime warning.The
test_plot_warns_if_xcol_is_datetimefunction effectively tests the warning mechanism for datetime-like x-axis columns. The use ofmocker.patchto capture the warning is a good approach. This test ensures that users are properly guided towards using the appropriate module for time-based plots.
105-131: LGTM: Thorough test for legend positioning.The
test_plot_moves_legend_outsidefunction effectively tests the legend positioning functionality. It verifies that thestandard_graph_stylesfunction is called with the correct parameters, includingmove_legend_outside=True. This approach ensures that the legend positioning option is correctly passed through theline.plotfunction.
134-160: LGTM: Consistent test for internal legend positioning.The
test_plot_moves_legend_insidefunction complements the previous test by verifying the behavior when the legend is positioned inside the plot. It maintains consistency in structure and assertions with the previous test, ensuring that both legend positioning options are thoroughly tested.
163-180: LGTM: Effective test for adding source text.The
test_plot_adds_source_textfunction properly tests the addition of source text to the plot. It verifies that theadd_source_textfunction is called with the correct parameters, ensuring that this feature of theline.plotfunction works as expected.
183-210: LGTM: Good test for legend title functionality.The
test_plot_with_legend_titlefunction effectively tests the application of a legend title. It verifies that thestandard_graph_stylesfunction is called with the correct parameters, including the specified legend title. This ensures that the legend title option in theline.plotfunction works as expected.
213-241: LGTM: Comprehensive test for combined legend options.The
test_plot_with_legend_title_and_move_outsidefunction effectively tests the combination of legend title and outside positioning options. It verifies that thestandard_graph_stylesfunction is called with the correct parameters for both options. This thorough test ensures that multiple legend customization options work correctly together in theline.plotfunction.
244-271: LGTM: Effective test for datetime index warning.The
test_plot_with_datetime_index_warnsfunction properly tests the warning mechanism for DataFrames with a datetime index when no x_col is specified. The use ofmocker.patchto capture the warning is appropriate. The test creates a new DataFrame with a datetime index, which is a good approach to simulate this scenario. This ensures that users are properly guided towards using the appropriate module for time-based plots when working with datetime indices.
pyretailscience/plots/line.py
Outdated
|
|
||
| elif x_col is None and pd.api.types.is_datetime64_any_dtype(df.index): | ||
| warnings.warn( | ||
| "The DataFrame index is datetime-like. Consider using the 'plots.timeline' module for time-based plots.", |
There was a problem hiding this comment.
It should be time_line (ie 2 words). Can you please do a quick find/replace
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
docs/analysis_modules.md (2)
16-16: Consider splitting the line for better readability.The line exceeds the recommended length of 120 characters. Consider splitting it into two lines for improved readability.
Here's a suggested change:
-Line plots are particularly good for visualizing sequences that are ordered or sequential, but not necessarily categorical, such as: +Line plots are particularly good for visualizing sequences that are ordered or sequential, +but not necessarily categorical, such as:Tools
Markdownlint
16-16: Expected: 120; Actual: 132
Line length(MD013, line-length)
22-22: Consider splitting the line for better readability.The line exceeds the recommended length of 120 characters. Consider splitting it into two lines for improved readability.
Here's a suggested change:
-They are often used to compare trends across categories, show the impact of events on performance, and visualize changes over time-like sequences. +They are often used to compare trends across categories, show the impact of events on performance, +and visualize changes over time-like sequences.Tools
Markdownlint
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
tests/plots/test_line.py (1)
15-15: Use@pytest.fixturewithout parenthesesWhen no parameters are passed to
@pytest.fixture, it's recommended to omit the parentheses for cleaner code.Apply this diff to fix the issue:
-@pytest.fixture() +@pytest.fixtureAlso applies to: 26-26, 33-33
Tools
Ruff
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- docs/analysis_modules.md (1 hunks)
- pyretailscience/plots/line.py (1 hunks)
- tests/plots/test_line.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyretailscience/plots/line.py
Additional context used
Markdownlint
docs/analysis_modules.md
16-16: Expected: 120; Actual: 132
Line length(MD013, line-length)
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
24-24: Expected: 120; Actual: 227
Line length(MD013, line-length)
Ruff
tests/plots/test_line.py
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
26-26: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
33-33: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
Additional comments not posted (2)
docs/analysis_modules.md (2)
10-52: LGTM! Well-structured and informative section on Line Plots.The new section on Line Plots is well-written and provides valuable information about the use cases and implementation of line plots using the
pyretailsciencelibrary. The example is clear and demonstrates the usage of theline.plotfunction effectively.Tools
Markdownlint
16-16: Expected: 120; Actual: 132
Line length(MD013, line-length)
22-22: Expected: 120; Actual: 146
Line length(MD013, line-length)
24-24: Expected: 120; Actual: 227
Line length(MD013, line-length)
24-24: LGTM! Previously suggested changes have been implemented.The line length issue for this note has already been addressed based on a previous review comment. The current wording is clear and informative.
Tools
Markdownlint
24-24: Expected: 120; Actual: 227
Line length(MD013, line-length)
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
pyretailscience/plots/line.py (3)
1-34: Enhance module docstring clarity and consistencyThe module docstring is comprehensive and well-structured. However, consider the following improvements:
- Ensure consistency in referring to the "timeline" module. Currently, it's mentioned as both "timeline" and "time_line".
- Consider making the warning about datetime columns more prominent, perhaps by using bold text or moving it to a separate "Important Note" section.
- The "Limitations and Handling of Temporal Data" section could be more concise. Consider merging the two bullet points into a single, more focused explanation.
These changes will improve readability and help users quickly grasp the module's capabilities and limitations.
36-42: Remove unused importThe
get_base_cmapfunction is imported frompyretailscience.style.tailwindbut is not used in the code. Consider removing this unused import to keep the module clean and prevent potential confusion.Apply this diff to remove the unused import:
-from pyretailscience.style.tailwind import get_base_cmap
59-77: Enhance function docstring clarity and completenessThe function docstring is informative, but consider the following improvements:
- Add more details about the
value_colparameter, explaining that it can be a single column name or a list of column names.- Clarify that
x_coldefaults to the DataFrame index if not provided.- Mention that
group_colis used to create multiple lines in the plot.- Add a note about the
**kwargsparameter, explaining that it accepts additional plotting parameters.These changes will provide users with a more comprehensive understanding of the function's capabilities and usage.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pyretailscience/plots/line.py (1 hunks)
Additional comments not posted (1)
pyretailscience/plots/line.py (1)
45-58: 🛠️ Refactor suggestionImprove default values for optional parameters
The function signature is well-defined with appropriate type hints. However, consider the following improvements:
- Use
Noneas the default value forx_label,y_label, andtitleinstead of empty strings. This allows for easier conditional rendering of these elements.- Consider adding a default value for
value_colto make it a required parameter, as it's essential for the function's operation.Apply this diff to improve the function signature:
def plot( df: pd.DataFrame, - value_col: str | list[str], + value_col: str | list[str], # No default, making it a required parameter - x_label: str, - y_label: str, - title: str, + x_label: str | None = None, + y_label: str | None = None, + title: str | None = None, x_col: str | None = None, group_col: str | None = None, ax: Axes | None = None, source_text: str | None = None, legend_title: str | None = None, move_legend_outside: bool = False, **kwargs: dict[str, any], ) -> SubplotBase:Likely invalid or redundant comment.
|
LGTM! |
PR Type
Enhancement, Documentation
Description
line.pyfor creating line plots from pandas DataFrames, focusing on sequences resembling time-based data.Changes walkthrough 📝
line.py
Add line plotting functionality with customization optionspyretailscience/plots/line.py
data.
Summary by CodeRabbit