Prevent unexpected alpha/beta returns from _calc_alpha_beta...#4
Prevent unexpected alpha/beta returns from _calc_alpha_beta...#4
Conversation
…ng if an array has all equal elements and returning the scalar instead
Reviewer's GuideThis PR updates _calc_alpha_beta to collapse uniform-array outputs into scalars with optional debug logging, adds explicit type annotations, extends test coverage for the new behavior, and bumps the project version. Class diagram for updated _calc_alpha_beta functionclassDiagram
class _calc_alpha_beta {
+mini: np.float64 | np.ndarray | float | int
+mode: np.float64 | np.ndarray | float | int
+maxi: np.float64 | np.ndarray | float | int
+lambd: np.float64 | np.ndarray | float | int
+_calc_alpha_beta(mini, mode, maxi, lambd) -> tuple[np.float64 | np.ndarray | float | int, np.float64 | np.ndarray | float | int]
}
_calc_alpha_beta : checks if alpha and beta are arrays with all equal elements
_calc_alpha_beta : returns scalar if arrays are uniform
_calc_alpha_beta : emits debug logs if DEBUG is set and unexpected arrays are encountered
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Method
participant Funcs as _calc_alpha_beta
participant Debug as Debug Logic
Test->>Funcs: Call with (mini, mode, maxi, lambd)
alt DEBUG enabled
Funcs->>Debug: Check if inputs are numpy arrays
Debug-->>Funcs: Write diagnostic if arrays detected
end
Funcs->>Funcs: Compute alpha, beta
alt alpha and beta are arrays and all elements equal
Funcs->>Test: Return first element as scalars
else alpha and beta are arrays with differing elements
alt DEBUG enabled
Funcs->>Debug: Write warning to stderr
end
Funcs->>Test: Return arrays
else
Funcs->>Test: Return scalars
end
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @hbmartin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refines the behavior of the _calc_alpha_beta function within the betapert library. The primary goal is to ensure that when array inputs to this function would logically result in scalar outputs (i.e., all elements of the resulting alpha and beta arrays are identical), the function correctly returns scalar values instead of arrays. This change improves the predictability and consistency of the function's output, and is supported by new comprehensive tests and a package version bump.
Highlights
- _calc_alpha_beta behavior refinement: Modified the
_calc_alpha_betafunction to detect if the calculatedalphaandbetaare NumPy arrays where all elements are identical. If so, it now returns the scalar value (the first element) instead of the array, preventing unexpected array returns when a scalar is implied. - Type Hinting: Added explicit type hints to the
_calc_alpha_betafunction signature, improving code readability and maintainability by specifying expected input and output types (float, int, or NumPy arrays). - Test Coverage: Introduced new unit tests to specifically validate the updated behavior of
_calc_alpha_beta, covering scenarios where array inputs result in either scalar or array outputs based on element equality. - Version Update: Incremented the package version in
pyproject.tomlfrom0.1.9to0.2.0, indicating a new release with this functional improvement.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like
Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @hbmartin - I've reviewed your changes - here's some feedback:
- The DEBUG check for array inputs currently uses mixed
and/orwithout full grouping—consider usingall(isinstance(...))or parentheses to ensure you only log when all four inputs are arrays. - Avoid writing debug messages directly to stderr; it would be better to use Python’s logging module with a dedicated logger to control verbosity in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The DEBUG check for array inputs currently uses mixed `and/or` without full grouping—consider using `all(isinstance(...))` or parentheses to ensure you only log when all four inputs are arrays.
- Avoid writing debug messages directly to stderr; it would be better to use Python’s logging module with a dedicated logger to control verbosity in production.
## Individual Comments
### Comment 1
<location> `betapert/funcs.py:107` </location>
<code_context>
alpha = 1 + ((mode - mini) * lambd) / (maxi - mini)
beta = 1 + ((maxi - mode) * lambd) / (maxi - mini)
+ # If alpha and beta are arrays and all elements are equal, return the scalar value
+ if DEBUG and (
+ isinstance(mini, np.ndarray)
+ and isinstance(mode, np.ndarray)
+ or isinstance(maxi, np.ndarray)
+ or isinstance(lambd, np.ndarray)
+ ):
+ sys.stderr.write(
</code_context>
<issue_to_address>
Logical grouping in the condition may not match intent.
Mixing 'and' and 'or' without parentheses can cause unintended logic. Add parentheses to clarify the intended evaluation order.
</issue_to_address>
### Comment 2
<location> `betapert/funcs.py:116` </location>
<code_context>
+ sys.stderr.write(
+ f"CAB: unexpected array parameters: mini={mini}, mode={mode}, maxi={maxi}, lambd={lambd}\n"
+ )
+ if isinstance(alpha, np.ndarray) and isinstance(beta, np.ndarray):
+ if np.all(alpha == alpha.flat[0]) and np.all(beta == beta.flat[0]):
+ return alpha[0], beta[0]
+ elif DEBUG:
</code_context>
<issue_to_address>
Returning alpha[0], beta[0] may not always yield scalars.
If alpha and beta are multi-dimensional arrays, alpha[0] and beta[0] may still be arrays. Use .item() to ensure scalars are returned.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if np.all(alpha == alpha.flat[0]) and np.all(beta == beta.flat[0]):
return alpha[0], beta[0]
=======
if np.all(alpha == alpha.flat[0]) and np.all(beta == beta.flat[0]):
return alpha.item(), beta.item()
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `betapert/funcs.py:86` </location>
<code_context>
-def _calc_alpha_beta(mini, mode, maxi, lambd):
+def _calc_alpha_beta(
+ mini: np.float64 | np.ndarray | float | int,
+ mode: np.float64 | np.ndarray | float | int,
</code_context>
<issue_to_address>
Consider extracting array handling and uniformity checks into a helper function and simplifying type hints to streamline the function.
```suggestion
Rather than sprinkling `DEBUG`‐branch logs and nested `isinstance` checks inside `_calc_alpha_beta`, you can:
1. Collapse the array-check into a single `any(...)` call.
2. Extract the “uniform array → scalar” logic into a tiny helper that uses `np.squeeze`/`.item()`.
3. Optionally simplify your type hints to `ArrayLike` from `numpy.typing`.
Example:
```python
from numpy.typing import ArrayLike
def _flatten_if_uniform(alpha: ArrayLike, beta: ArrayLike):
# squeeze out singleton dims
a0, b0 = np.squeeze(alpha), np.squeeze(beta)
# if still 0-d arrays, convert to Python scalars
return (a0.item(), b0.item()) if np.isscalar(a0) and np.isscalar(b0) else (alpha, beta)
def _calc_alpha_beta(
mini: ArrayLike,
mode: ArrayLike,
maxi: ArrayLike,
lambd: ArrayLike,
) -> tuple[ArrayLike, ArrayLike]:
alpha = 1 + ((mode - mini) * lambd) / (maxi - mini)
beta = 1 + ((maxi - mode) * lambd) / (maxi - mini)
if DEBUG and any(isinstance(x, np.ndarray) for x in (mini, mode, maxi, lambd)):
sys.stderr.write(f"CAB: array params mini={mini}, mode={mode}, maxi={maxi}, lambd={lambd}\n")
return _flatten_if_uniform(alpha, beta)
```
This keeps all functionality, collapses branching, and makes the core logic much tighter.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if np.all(alpha == alpha.flat[0]) and np.all(beta == beta.flat[0]): | ||
| return alpha[0], beta[0] |
There was a problem hiding this comment.
suggestion (bug_risk): Returning alpha[0], beta[0] may not always yield scalars.
If alpha and beta are multi-dimensional arrays, alpha[0] and beta[0] may still be arrays. Use .item() to ensure scalars are returned.
| if np.all(alpha == alpha.flat[0]) and np.all(beta == beta.flat[0]): | |
| return alpha[0], beta[0] | |
| if np.all(alpha == alpha.flat[0]) and np.all(beta == beta.flat[0]): | |
| return alpha.item(), beta.item() |
|
|
||
|
|
||
| def _calc_alpha_beta(mini, mode, maxi, lambd): | ||
| def _calc_alpha_beta( |
There was a problem hiding this comment.
issue (complexity): Consider extracting array handling and uniformity checks into a helper function and simplifying type hints to streamline the function.
| def _calc_alpha_beta( | |
| Rather than sprinkling `DEBUG`‐branch logs and nested `isinstance` checks inside `_calc_alpha_beta`, you can: | |
| 1. Collapse the array-check into a single `any(...)` call. | |
| 2. Extract the “uniform array → scalar” logic into a tiny helper that uses `np.squeeze`/`.item()`. | |
| 3. Optionally simplify your type hints to `ArrayLike` from `numpy.typing`. | |
| Example: | |
| ```python | |
| from numpy.typing import ArrayLike | |
| def _flatten_if_uniform(alpha: ArrayLike, beta: ArrayLike): | |
| # squeeze out singleton dims | |
| a0, b0 = np.squeeze(alpha), np.squeeze(beta) | |
| # if still 0-d arrays, convert to Python scalars | |
| return (a0.item(), b0.item()) if np.isscalar(a0) and np.isscalar(b0) else (alpha, beta) | |
| def _calc_alpha_beta( | |
| mini: ArrayLike, | |
| mode: ArrayLike, | |
| maxi: ArrayLike, | |
| lambd: ArrayLike, | |
| ) -> tuple[ArrayLike, ArrayLike]: | |
| alpha = 1 + ((mode - mini) * lambd) / (maxi - mini) | |
| beta = 1 + ((maxi - mode) * lambd) / (maxi - mini) | |
| if DEBUG and any(isinstance(x, np.ndarray) for x in (mini, mode, maxi, lambd)): | |
| sys.stderr.write(f"CAB: array params mini={mini}, mode={mode}, maxi={maxi}, lambd={lambd}\n") | |
| return _flatten_if_uniform(alpha, beta) |
This keeps all functionality, collapses branching, and makes the core logic much tighter.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where _calc_alpha_beta could return arrays with identical elements instead of a scalar. The added logic to check for uniform arrays and return a scalar is a good fix, and the new tests validate this behavior correctly.
| sys.stderr.write( | ||
| f"CAB: unexpected array parameters: mini={mini}, mode={mode}, maxi={maxi}, lambd={lambd}\n" | ||
| ) | ||
| if isinstance(alpha, np.ndarray) and isinstance(beta, np.ndarray): |
There was a problem hiding this comment.
This check for isinstance(alpha, np.ndarray) and isinstance(beta, np.ndarray) is slightly redundant. Due to NumPy's broadcasting, if any of the input parameters is an array, both alpha and beta will be np.ndarrays. Checking only one of them is sufficient.
if isinstance(alpha, np.ndarray):![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to d9ee844 in 2 minutes and 4 seconds. Click for details.
- Reviewed
95lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. betapert/funcs.py:87
- Draft comment:
The function signature and type annotations now allow array returns, but the docstring still mentions a tuple of floats. Update the docstring to reflect that scalars or arrays can be returned. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. betapert/funcs.py:113
- Draft comment:
Consider replacing sys.stderr.write debug prints with the logging module for better configurability and standardized logging. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. tests/test_ppf_log_fallback.py:315
- Draft comment:
In the scalar fallback test, consider using np.isscalar(result_scalar) instead of isinstance(result_scalar, np.number) to more clearly check for a scalar value. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
Workflow ID: wflow_Xq63lsCr7pJ3SbMm
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
betapert/funcs.py(2 hunks)pyproject.toml(1 hunks)tests/test_ppf_log_fallback.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pytest, ruff, and black
tests/test_ppf_log_fallback.py
[error] 372-372: Ruff PLC0415: import should be at the top-level of a file.
[error] 372-373: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
[error] 373-373: Ruff PLC0415: import should be at the top-level of a file.
[error] 389-389: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
[error] 389-390: Ruff PLC0415: import should be at the top-level of a file.
[error] 390-390: Ruff PLC0415: import should be at the top-level of a file.
betapert/funcs.py
[error] 87-90: Ruff PYI041: Use float instead of int | float. Remove redundant type annotations in function parameters.
[error] 108-108: Ruff RUF021: Parenthesize a and b expressions when chaining and and or together to clarify precedence.
[error] 114-114: Ruff E501: Line too long (103 > 100).
[error] 114-114: Ruff COM812: Trailing comma missing.
[error] 119-119: Ruff RET505: Unnecessary elif after return statement. Remove unnecessary elif.
🔇 Additional comments (3)
pyproject.toml (1)
4-4: Version bump looks appropriate for the feature addition.The minor version increment from 0.1.9 to 0.2.0 correctly reflects the addition of new conditional logic in
_calc_alpha_betafunction.betapert/funcs.py (1)
104-121: The core logic for conditional scalar/array returns is well-implemented.The implementation correctly identifies when alpha and beta arrays have uniform values and returns scalars in those cases, which addresses the PR objective of preventing unexpected array returns.
tests/test_ppf_log_fallback.py (1)
370-402: Test logic correctly validates the new conditional behavior.Both tests properly verify that
_calc_alpha_betareturns scalars when input arrays have uniform elements and returns arrays when elements differ, which aligns with the PR objective.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 60d5e8f in 2 minutes and 26 seconds. Click for details.
- Reviewed
95lines of code in3files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. betapert/funcs.py:87
- Draft comment:
Type hints now exclude 'int'. If integer values are expected, consider including them (or document that ints will be accepted via implicit conversion). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. betapert/funcs.py:110
- Draft comment:
Using alpha[0] assumes a 1D array. For multi-dimensional arrays, consider using alpha.flat[0] to ensure a true scalar is returned when all elements are equal. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Looking at the code, this is inside _calc_alpha_beta which takes numpy arrays as input. The alpha/beta calculations are simple arithmetic operations that preserve array shape. For multi-dimensional arrays, both [0] and flat[0] would work - [0] gets the first element along the first axis, flat[0] gets the first element in flattened order. Since we're checking np.all(alpha == alpha[0]), we're already verifying all elements are equal, so the specific access method doesn't matter. The comment raises a valid point about multi-dimensional arrays behaving differently. Maybe there's an edge case where the array elements are equal but in different order between flattened and non-flattened views? Since np.all() is checking element-wise equality with alpha[0], if all elements are equal, they must be equal regardless of array shape or access method. The order doesn't matter when all elements are the same value. The comment suggests a change that would be functionally equivalent and not fix any real issues. The current code is correct as-is.
3. betapert/funcs.py:107
- Draft comment:
Rather than using sys.stderr.write for debug logging, consider using the logging module for better configurability and integration. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
4. tests/test_ppf_log_fallback.py:315
- Draft comment:
Using isinstance(result_scalar, np.number) may not catch native Python floats. Consider using np.isscalar(result_scalar) to check for scalar values. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_kfzJIL4MpX26WF1C
You can customize
1 participant
User description
...by checking if an array has all equal elements and returning the scalar instead
PR Type
Bug fix, Enhancement
Description
Fix unexpected array returns from
_calc_alpha_betafunctionAdd type hints and debug logging for array parameters
Return scalar values when all array elements are equal
Bump version to 0.2.0
Diagram Walkthrough
File Walkthrough
funcs.py
Fix array handling in _calc_alpha_beta functionbetapert/funcs.py
_calc_alpha_betafunction parameters and return typetest_ppf_log_fallback.py
Add tests for _calc_alpha_beta array handlingtests/test_ppf_log_fallback.py
_calc_alpha_betawith array arguments having all equalelements
_calc_alpha_betawith array arguments having differentelements
pyproject.toml
Version bump to 0.2.0pyproject.toml
Summary by CodeRabbit
New Features
Tests
Chores
Important
Fix
_calc_alpha_betato return scalars for all-equal arrays, add type hints, logging, and tests, and update version to 0.2.0._calc_alpha_betainfuncs.pyto return scalars when all array elements are equal._calc_alpha_beta.test_ppf_log_fallback.pyfor_calc_alpha_betawith all-equal and non-equal array elements.pyproject.toml.This description was created by
for 60d5e8f. You can customize this summary. It will automatically update as commits are pushed.