fix: gain loss calc error with negative values

PR Type

Bug fix, Tests, Documentation

Description

Added detailed documentation to pyretailscience/gain_loss.py to explain gain loss analysis.
Introduced a new method process_customer_group to handle customer group processing in a more modular way.
Refactored the _calc_gain_loss method to utilize the new process_customer_group method, improving code readability and maintainability.
Improved error messages for better clarity.
Added type hints to method signatures for better code understanding.
Removed outdated tests and sample data from tests/test_gain_loss.py.
Added comprehensive parameterized tests for the process_customer_group method to ensure correctness.

Changes walkthrough 📝

Relevant files

Enhancement

gain_loss.py

Refactor gain loss calculation and add documentation

pyretailscience/gain_loss.py

Added detailed module-level docstring explaining gain loss analysis.

Introduced process_customer_group method to handle customer group
processing.

Refactored _calc_gain_loss method to use process_customer_group.

Improved error messages and added type hints.

+99/-25

Tests

test_gain_loss.py

Add parameterized tests for process_customer_group method

tests/test_gain_loss.py

Removed old tests and sample data.

Added parameterized tests for process_customer_group.

+60/-148

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

Summary by CodeRabbit

New Features
- Added a new static method process_customer_group to the GainLoss class for detailed gain loss calculations on customer groups.
Refactor
- Refactored the _calc_gain_loss method to utilize the new process_customer_group method.
Tests
- Enhanced test cases to cover various scenarios for the process_customer_group function, ensuring robust validation of its behavior.

Walkthrough

The GainLoss class in pyretailscience/gain_loss.py has been enhanced to explain gain loss analysis in detail. The __init__ method now explicitly returns None, and a new static method, process_customer_group, has been introduced to refine gain loss calculations. Corresponding updates in _calc_gain_loss utilize this new method. Test cases in tests/test_gain_loss.py have also been updated to validate various scenarios, ensuring robust functionality.

Changes

File	Change Summary
`pyretailscience/gain_loss.py`	Added detailed explanation, return type to `__init__`, and `process_customer_group` method. Refactored `_calc_gain_loss` to use the new method.
`tests/test_gain_loss.py`	Enhanced test cases for `process_customer_group`, covering diverse scenarios and validating various conditions.

Poem

In the realm of code so fine, 🐇
Gain and loss, we now define. 📊
A static method, clear and bright, 🌟
Processes groups with calculating might! 💻
Tests ensure no bugs take flight. 🚀
Every customer counts, that's right! 🌈

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?

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips

### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` 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 as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

**Refactoring:** The refactoring of `_calc_gain_loss` to use `process_customer_group` should be thoroughly tested to ensure that the logic for calculating gains and losses remains correct. **Documentation:** Ensure that the newly added documentation in `gain_loss.py` is clear and accurately describes the functionality and usage of the module and methods.

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Remove the invalid `strict=False` parameter from the `zip` function ___ The `strict=False` parameter in the `zip` function is not necessary and should be removed to avoid confusion, as it is not a valid parameter for `zip`. [pyretailscience/gain_loss.py [226]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-bcf245edc79a21d026c9a33bdef67631b8eee1219f3552da55e900906ba27223R226-R226) ```diff -strict=False, +# Removed line ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 10 Why: This suggestion correctly points out an invalid parameter in the `zip` function call, which could potentially lead to confusion or errors. Removing it avoids possible issues in the future.	10
	Add test cases with invalid or edge-case inputs to ensure robust handling of such scenarios ___ To ensure that the `GainLoss.process_customer_group` method handles invalid inputs gracefully, add test cases with invalid or edge-case inputs such as negative values for `focus_p1` and `comparison_p1`. [tests/test_gain_loss.py [9]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-942ce7636d093d390ac7428ee0da4be409652ca1d975f6ca6a826b926bd8a6f4R9-R9) ```diff -(0, 0, 100, 0, 100, 0, (100, 0, 0, 0, 0, 0)), +(0, -10, 100, 0, 100, 0, (100, 0, 0, 0, 0, 0)), ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding test cases for invalid or edge-case inputs like negative values is crucial for ensuring the robustness of the method under test.	8
	Ensure that `focus_p1`, `comparison_p1`, `focus_p2`, and `comparison_p2` are non-negative before processing ___ Add a check to ensure that `focus_p1`, `comparison_p1`, `focus_p2`, and `comparison_p2` are non-negative before processing, as negative values might indicate data issues. [pyretailscience/gain_loss.py [122]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-bcf245edc79a21d026c9a33bdef67631b8eee1219f3552da55e900906ba27223R122-R122) ```diff +if any(val < 0 for val in [focus_p1, comparison_p1, focus_p2, comparison_p2]): + raise ValueError("focus_p1, comparison_p1, focus_p2, and comparison_p2 must be non-negative") if focus_p1 == 0 and comparison_p1 == 0: ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential data integrity issue by ensuring that input values are non-negative before processing. This is crucial for maintaining the accuracy and reliability of the gain loss calculations.	8
	Add a test case for the scenario where both differences are zero but initial and final values differ ___ Add a test case to cover the scenario where both `focus_diff` and `comparison_diff` are zero, but the initial and final values are different. This will ensure that the function handles such edge cases correctly. [tests/test_gain_loss.py [37]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-942ce7636d093d390ac7428ee0da4be409652ca1d975f6ca6a826b926bd8a6f4R37-R37) ```diff -(100, 100, 100, 100, 0, 0, (0, 0, 0, 0, 0, 0)), +(50, 100, 50, 100, 0, 0, (0, 0, 0, 0, 0, 0)), ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding a test case to cover scenarios where differences are zero but initial and final values differ is a good practice to catch edge cases, although the existing tests already cover similar scenarios.	6
Best practice	Use more descriptive variable names in the parameterized test cases to improve readability ___ To improve readability and maintainability, consider using descriptive variable names for the parameters in the `@pytest.mark.parametrize` decorator. This will make the test cases easier to understand at a glance. [tests/test_gain_loss.py [6]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-942ce7636d093d390ac7428ee0da4be409652ca1d975f6ca6a826b926bd8a6f4R6-R6) ```diff -"focus_p1, comparison_p1, focus_p2, comparison_p2, focus_diff, comparison_diff, expected", +"focus_period1, comparison_period1, focus_period2, comparison_period2, focus_difference, comparison_difference, expected_result", ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to use more descriptive variable names in the parameterized test cases is valid and would improve code readability and maintainability.	7
Best practice	Rename the test function to be more descriptive ___ To improve the clarity of the test function, consider renaming `test_process_customer_group` to something more descriptive, such as `test_gain_loss_process_customer_group`. [tests/test_gain_loss.py [54]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-942ce7636d093d390ac7428ee0da4be409652ca1d975f6ca6a826b926bd8a6f4R54-R54) ```diff -def test_process_customer_group( +def test_gain_loss_process_customer_group( ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 5 Why: Renaming the test function to be more descriptive could help improve clarity, but the current name is already quite clear about what the function tests.	5
Maintainability	Simplify the multiplication operation by using a unary minus for consistency ___ *The check for `focus_p2 == 0 and comparison_p2 == 0` should return `0, -focus_p1, 0, 0, 0, 0` instead of `0, -1 focus_p1, 0, 0, 0, 0` for consistency and readability.** [pyretailscience/gain_loss.py [125]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-bcf245edc79a21d026c9a33bdef67631b8eee1219f3552da55e900906ba27223R125-R125) ```diff -return 0, -1 * focus_p1, 0, 0, 0, 0 +return 0, -focus_p1, 0, 0, 0, 0 ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies a minor readability improvement in the code by simplifying the multiplication operation. This change makes the code slightly more readable and consistent.	7
Performance	Store the result of `comparison_diff + focus_diff` in a variable to avoid redundant calculations ___ Instead of using `max(0, comparison_diff + focus_diff)` and `min(0, comparison_diff +` `focus_diff)`, consider storing the result of `comparison_diff + focus_diff` in a variable to avoid redundant calculations. [pyretailscience/gain_loss.py [128-132]](https://github.com/Data-Simply/pyretailscience/pull/56/files#diff-bcf245edc79a21d026c9a33bdef67631b8eee1219f3552da55e900906ba27223R128-R132) ```diff -focus_inc_dec = focus_diff if comparison_diff > 0 else max(0, comparison_diff + focus_diff) +combined_diff = comparison_diff + focus_diff +focus_inc_dec = focus_diff if comparison_diff > 0 else max(0, combined_diff) ... -focus_inc_dec = min(0, comparison_diff + focus_diff) +focus_inc_dec = min(0, combined_diff) ``` Suggestion importance[1-10]: 6 Why: The suggestion improves the performance by reducing redundant calculations, which is a good practice for cleaner and potentially faster code. However, the impact on performance might be minor given the context.	6

/describe --pr_description.publish_labels=true --pr_description.publish_description_as_comment=true --pr_description.generate_ai_title=true

Title

Refactor gain/loss calculation and add parameterized tests

PR Type

Bug fix, Tests