Feature/refactor standard graph plots #93
Conversation
WalkthroughThe changes replace the Poetry-based dependency management in GitHub workflows with the Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)
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 (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
pyretailscience/plots/index.py (1)
58-59: Validate aggregator string usage.Currently, the
agg_funcparameter is taken as a string (e.g.,"sum") without verifying if it is recognized by Pandas. Consider validating or handling invalid aggregator inputs to prevent runtime errors.tests/plots/test_time.py (1)
24-24: Remove parentheses in @pytest.fixture.According to the style hint (PT001), use
@pytest.fixtureinstead of@pytest.fixture().- @pytest.fixture() + @pytest.fixture🧰 Tools
🪛 Ruff (0.8.2)
24-24: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
tests/plots/test_waterfall.py (2)
15-15: Remove parentheses in @pytest.fixture.According to the style hint (PT001), use
@pytest.fixtureinstead of@pytest.fixture().- @pytest.fixture() + @pytest.fixture🧰 Tools
🪛 Ruff (0.8.2)
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
54-54: Cleanup function name to remove duplication.The method name
test_plot_colors_assigned_correctly_replicated_replicatedappears to contain a duplicated word. Consider renaming it to something liketest_plot_colors_assigned_correctly.pyretailscience/plots/time.py (2)
46-48: Address the TODO comment about color range.The TODO comment suggests simplifying the color range in the
get_linear_cmapfunction.Would you like me to help implement this simplification or create an issue to track this task?
51-148: Consider enhancing error handling and input validation.The
plotfunction could benefit from additional error handling:
- Validate that
value_colexists in the DataFrame- Validate that
periodis a valid time period string- Validate that
agg_funcis a valid aggregation functionHere's a suggested implementation:
def plot( df: pd.DataFrame, value_col: str, period: str | BaseOffset = "D", agg_func: str = "sum", group_col: str | None = None, ... ) -> SubplotBase: + if value_col not in df.columns: + raise ValueError(f"Column '{value_col}' not found in DataFrame") + + if isinstance(period, str) and period not in ["D", "W", "M", "Q", "Y"]: + raise ValueError(f"Invalid period '{period}'. Must be one of: D, W, M, Q, Y") + + if agg_func not in ["sum", "mean", "count", "min", "max"]: + raise ValueError(f"Invalid aggregation function '{agg_func}'") + + if group_col is not None and group_col not in df.columns: + raise ValueError(f"Column '{group_col}' not found in DataFrame") df["transaction_period"] = df[get_option("column.transaction_date")].dt.to_period( period, ) ...pyretailscience/plots/waterfall.py (1)
83-94: Consider improving error message for data label format validation.The error message could be more descriptive by listing the valid options.
- "data_label_format must be either 'absolute', 'percentage', 'both', or None.", + "data_label_format must be one of: 'absolute', 'percentage', 'both', or None",tests/plots/test_index.py (1)
134-134: Remove unnecessary parentheses in pytest.fixture decorators.Static analysis suggests removing empty parentheses from pytest.fixture decorators.
-@pytest.fixture() +@pytest.fixtureAlso applies to: 145-145
🧰 Tools
🪛 Ruff (0.8.2)
134-134: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/workflows/gh-page.yml(1 hunks).github/workflows/release.yml(2 hunks).github/workflows/test.yml(1 hunks)docs/analysis_modules.md(4 hunks)pyproject.toml(1 hunks)pyretailscience/plots/index.py(1 hunks)pyretailscience/plots/time.py(1 hunks)pyretailscience/plots/waterfall.py(1 hunks)pyretailscience/standard_graphs.py(0 hunks)pyretailscience/style/graph_utils.py(3 hunks)tests/plots/test_index.py(1 hunks)tests/plots/test_time.py(1 hunks)tests/plots/test_waterfall.py(1 hunks)tests/test_standard_graphs.py(0 hunks)
💤 Files with no reviewable changes (2)
- tests/test_standard_graphs.py
- pyretailscience/standard_graphs.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/plots/test_time.py
24-24: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
tests/plots/test_waterfall.py
15-15: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
tests/plots/test_index.py
134-134: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
145-145: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
🔇 Additional comments (18)
pyretailscience/plots/index.py (1)
264-265: Good validation on df_index_filter.Raising an error when the filter is all True or all False helps avoid misleading calculations. This check is clear and prevents unintended edge cases.
tests/plots/test_time.py (1)
1-23: Test coverage looks solid.All core scenarios (e.g., invalid period, group columns, source text) are covered comprehensively.
pyretailscience/plots/time.py (1)
1-34: LGTM! Well-structured documentation.The module's documentation is comprehensive and well-organized, covering features, use cases, limitations, and functionality details. This enhances code maintainability and usability.
pyretailscience/plots/waterfall.py (1)
1-28: LGTM! Well-structured documentation.The module's documentation is comprehensive and well-organized, providing clear explanations of features, use cases, and functionality.
tests/plots/test_index.py (2)
13-129: LGTM! Comprehensive test coverage for get_indexes().The test suite thoroughly covers various scenarios including single column, two columns, offset parameter, aggregation functions, and error cases.
131-305: LGTM! Comprehensive test coverage for plot().The test class provides thorough coverage of the plot functionality, including:
- Default parameters
- Custom titles and labels
- Highlight ranges
- Group filters
- Error handling
- Source text
- Legend behavior
🧰 Tools
🪛 Ruff (0.8.2)
134-134: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
145-145: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
pyretailscience/style/graph_utils.py (2)
178-178: LGTM! Well-documented parameter addition.The new
show_legendparameter is properly documented with a clear description of its purpose.Also applies to: 193-193
228-235: LGTM! Clean implementation of legend control.The implementation cleanly handles the legend display control by returning early when show_legend is False.
.github/workflows/gh-page.yml (1)
33-38: LGTM! Consistent with other workflow changes.The workflow has been updated to use uv instead of Poetry, maintaining consistency with other workflow files.
.github/workflows/release.yml (2)
37-43: LGTM! Consistent with other workflow changes.The workflow has been updated to use uv instead of Poetry, maintaining consistency with other workflow files.
59-59: LGTM! Version retrieval updated for uv.The version retrieval command has been correctly updated to use uv's version command.
pyproject.toml (4)
1-23: LGTM! Well-structured project metadata and dependencies.The project configuration has been updated to use the modern
[project]format with explicit version constraints for dependencies.
25-43: LGTM! Well-organized dependency groups.Dependencies are logically grouped into dev, examples, and docs categories with explicit version constraints.
45-50: LGTM! Proper uv configuration.The uv configuration correctly includes all dependency groups by default.
52-54: LGTM! Updated build system.The build system has been correctly updated to use Hatchling.
docs/analysis_modules.md (3)
171-171: LGTM! Updated import path for waterfall plot.The import path has been correctly updated to use the new module structure.
Also applies to: 176-176
215-215: LGTM! Updated import path for index plot.The import path has been correctly updated to use the new module structure.
Also applies to: 217-217
256-256: LGTM! Updated import path for time plot.The import path has been correctly updated to use the new module structure.
Also applies to: 272-272
.github/workflows/test.yml
Outdated
| - name: Install uv Package | ||
| run: | | ||
| pip install --upgrade pip | ||
| pip install poetry==1.3.2 | ||
| poetry config virtualenvs.create false | ||
| poetry install --no-interaction --with dev | ||
| pip install uv==0.5.30 | ||
| uv sync |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the uv package version.
The workflow has been updated to use uv instead of Poetry, which is a good choice for faster dependency resolution. However, let's verify if version 0.5.30 is the latest stable version.
🌐 Web query:
What is the latest stable version of uv?
💡 Result:
The latest stable version of uv as of February 12, 2025 is 0.[7]. This information is confirmed by multiple sources:
- Homebrew lists
uvversion stable as version stable [7] - PyPI also shows uv version stable [2]
- The Snapcraft store lists
astral-uvwithlatest/stableupdated on January [9]
For installation:
# Install via Homebrew
brew install uv
# Install via Snap
sudo snap install astral-uv --classic
# Install via PyPI
pip install uv==Citations:
- 1: https://github.com/astral-sh/uv/releases
- 2: https://pypi.org/project/uv/
- 3: https://docs.astral.sh/uv/concepts/python-versions/
- 4: https://snapcraft.io/astral-uv
- 5: https://astral.sh/blog/uv
- 6: https://github.com/astral-sh/uv/blob/main/CHANGELOG.md?featured_on=pythonbytes
- 7: https://formulae.brew.sh/formula/uv
- 8: https://www.uvexplorer.com/downloads/
- 9: https://snapcraft.io/install/astral-uv/rhel
Outdated uv Package Version Detected
The workflow file currently installs uv 0.5.30. However, based on our research, the latest stable version of uv as of February 2025 is 0.7. Please update the version accordingly.
- Update uv package version in
.github/workflows/test.ymlfrom 0.5.30 to 0.7.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/gh-page.yml(1 hunks).github/workflows/release.yml(2 hunks).github/workflows/test.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/gh-page.yml
- .github/workflows/test.yml
.github/workflows/release.yml
Outdated
| - name: Install uv Package | ||
| run: | | ||
| pip install --upgrade pip | ||
| pip install build | ||
| pip install poetry==1.3.2 | ||
| poetry config virtualenvs.create false | ||
| poetry install --no-interaction --with dev | ||
| poetry version ${{ inputs.release }} | ||
| pip install uv==0.5.30 | ||
| export UV_VENV_CREATE=false | ||
| uv sync | ||
| uv version |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify UV package version and configuration.
The UV package is pinned to version 0.5.30. Since this is a relatively new package manager, we should verify if this is the latest stable version and if there are any known issues.
Additionally, consider adding error handling for the UV commands:
pip install uv==0.5.30
export UV_VENV_CREATE=false
-uv sync
-uv version
+uv sync || exit 1
+uv version || exit 1🌐 Web query:
What is the latest stable version of UV package manager? Are there any known issues with version 0.5.30?
💡 Result:
Based on the search results:
The latest stable version of UV mentioned across sources is 0.5.28, released on GitHub[4]. This version includes bug fixes for virtual environment discovery, SSL handling in Git operations, and tar unpacking[4].
There's no mention of version 0.5.30 in any provided sources - this version either:
- Doesn't exist yet as of February 2025
- May be an unreleased/development version
- Could be a typo in the query
For recent known issues (as of latest documented releases):
- Installation challenges on ARMv7 Linux systems [4]
- Occasional permission errors during cache writes on Windows [4]
- Python version management conflicts when mixing system and managed interpreters [2][4]
- Ephemeral environment cleanup issues in CI/CD pipelines [4]
Developers should consult UV's GitHub releases page for real-time updates[4]. The project maintains an active issue tracker addressing reported bugs[9].
Citations:
- 1: https://blog.det.life/changing-package-manager-from-pip-to-uv-81ac3b5a037d?gi=981849596391
- 2: https://www.youtube.com/watch?v=JtR7EyMcaWU
- 3: https://www.publications.usace.army.mil/portals/76/publications/engineermanuals/em_385-1-1.pdf
- 4: https://github.com/astral-sh/uv/releases
- 5: https://www.saaspegasus.com/guides/uv-deep-dive/
- 6: https://packages.msys2.org/packages/
- 7: https://astral.sh/blog/uv
- 8: UV: Python package manager. irthomasthomas/undecidability#964
- 9: https://github.com/astral-sh/uv/actions/runs/8346432463
- 10: https://docs.astral.sh/uv/getting-started/installation/
UV Package Version Issue & Error Handling Improvements
- The web query confirms that the latest stable version is 0.5.28, and using
uv==0.5.30may reference an unreleased or mistyped version. - Consider pinning UV to version 0.5.28.
- Additionally, add error handling for the
uv syncanduv versioncommands (e.g., using|| exit 1) to catch failures during workflow execution.
📝 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.
| - name: Install uv Package | |
| run: | | |
| pip install --upgrade pip | |
| pip install build | |
| pip install poetry==1.3.2 | |
| poetry config virtualenvs.create false | |
| poetry install --no-interaction --with dev | |
| poetry version ${{ inputs.release }} | |
| pip install uv==0.5.30 | |
| export UV_VENV_CREATE=false | |
| uv sync | |
| uv version | |
| - name: Install uv Package | |
| run: | | |
| pip install --upgrade pip | |
| pip install build | |
| pip install uv==0.5.30 | |
| export UV_VENV_CREATE=false | |
| uv sync || exit 1 | |
| uv version || exit 1 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.pre-commit-config.yaml (1)
22-22: LGTM! Consider pinning the pytest version.The integration with UV aligns well with the project's transition from Poetry. However, to ensure consistent test execution across all environments, consider pinning the pytest version.
- entry: uv run pytest --cov=pyretailscience --cov-report=xml --cov-branch tests + entry: uv run pytest==8.0.0 --cov=pyretailscience --cov-report=xml --cov-branch testsLet's verify the latest stable version of pytest:
✅ Verification successful
🌐 Web query:
What is the latest stable version of pytest?💡 Result:
The latest stable version of pytest as of February 2025 is 8.3.4, released on December 1, 2024[1][2][4][6]. This version includes bug fixes for crashes related to directory layouts, assertion rewriting improvements, and stricter boolean handling in
pytest.approx[2][4].Key details:
- Python compatibility: Requires Python 3.8+ or PyPy3[1][5]
- Installation command:
This will install/upgrade to the latest stable version[5][6].pip install -U pytestRecent updates focus on:
- Improved ANSI code handling for colored output[2]
- Better cache directory permissions (defaulting to
rwxr-xr-x)[2]- Enhanced namespace package detection[2]
For production environments, verify installation with:
pytest --version # Output: pytest 8.3.4Citations:
- 1: https://docs.pytest.org/en/stable/backwards-compatibility.html
- 2: https://docs.pytest.org/en/stable/changelog.html
- 3: https://docs.pytest.org/en/stable/
- 4: https://github.com/pytest-dev/pytest/releases
- 5: https://docs.pytest.org/en/stable/getting-started.html
- 6: https://pypi.org/project/pytest/
- 7: https://docs.pytest.org/en/stable/announce/index.html
- 8: https://pypi.org/project/pytest-cov/2.3.0/
LGTM! Consider pinning the pytest version to ensure a consistent test environment.
The transition from Poetry to UV is correctly reflected by running pytest via
uv run. However, rather than pinning an older version (e.g.,pytest==8.0.0), you should update the command to pin the latest stable release (as of February 2025, pytest 8.3.4) to avoid any unexpected discrepancies.
- Update the hook entry to:
- entry: uv run pytest --cov=pyretailscience --cov-report=xml --cov-branch tests + entry: uv run pytest==8.3.4 --cov=pyretailscience --cov-report=xml --cov-branch tests
Codecov ReportAttention: Patch coverage is
|
mayurkmmt
force-pushed
the
feature/refactor-standard-graph-plots
branch
from
021d2ff to
8779a44
Compare
…ta-simply/pyretailscience into feature/refactor-standard-graph-plots
User description
Feature/refactor standard graph plots
PR Type
Enhancement, Tests, Dependencies
Description
Refactored plotting modules into separate files for better modularity.
index.py,time.py, andwaterfall.pyfor specific plot types.Enhanced testing coverage for all plotting modules.
index,time, andwaterfallplots.Replaced Poetry with UV for dependency management.
pyproject.tomlto reflect this change.Updated documentation to reflect new module structure.
Changes walkthrough 📝
4 files
Added module for index plot functionalityAdded module for timeline plot functionalityAdded module for waterfall chart functionalityEnhanced graph utility functions for legend visibility3 files
Added unit tests for index plot moduleAdded unit tests for timeline plot moduleAdded unit tests for waterfall plot module3 files
Updated workflow to use UV for dependency managementUpdated release workflow to use UV for versioningUpdated test workflow to use UV for dependency management1 files
Updated documentation to reflect new plotting modules1 files
Migrated from Poetry to UV for dependency management2 files
Summary by CodeRabbit
time_plotandwaterfall_plotfunctions, covering various scenarios and edge cases.index_plotfunctionality, validating behavior under multiple conditions.