Fix array dimension handling for x_normalized#3
Conversation
Reviewer's GuideThis PR fixes array dimension handling in the log-space PPF fallback by switching to flat indexing, adds tests for both scalar and array inputs to verify correct output shape and value constraints, and bumps the package version. Class diagram for updated log_cdf_eq function array handlingclassDiagram
class log_cdf_eq {
+log_cdf_eq(x_normalized)
# results: ndarray
}
log_cdf_eq : results.flat[i] assignment
log_cdf_eq : returns scalar or array depending on input
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. WalkthroughThe Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ 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 (
|
![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 addresses a critical bug related to array dimension handling within the betapert library's probability point function (PPF) fallback logic. The change ensures that the calculation correctly assigns values when scipy.stats.beta.ppf returns an array, preventing a ValueError and improving the robustness of the numerical computations. A new test case has been added to validate this fix, enhancing the overall stability of the library.
Highlights
- Bug Fix: Array Dimension Handling: Corrected an issue in
betapert/funcs.pywithin thelog_cdf_eqfunction's fallback mechanism. Previously,results[i] = mini + (maxi - mini) * x_normalizedcould lead to aValueErrorifx_normalizedwas an array (e.g., whenscipy.stats.beta.ppfreturned an array). This has been fixed by usingresults.flat[i] = ...to ensure correct assignment regardless ofx_normalized's shape, treating theresultsarray as a flat sequence for individual element assignment. - New Test Case: Added a new test
test_log_fallback_handles_beta_ppf_array_returnintests/test_ppf_log_fallback.py. This test specifically validates that the_ppf_fallback_log_spacefunction correctly handles cases wherescipy.stats.beta.ppfreturns an array, ensuring robustness for both scalar and array inputs and preventing theValueErrorthat the fix addresses. - Version Update: The project version in
pyproject.tomlhas been incremented from0.1.7to0.1.8, reflecting this bug fix.
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. ↩
PR Reviewer Guide 🔍(Review updated until commit 6dbd6aa)Here are some key observations to aid the review process:
|
![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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/test_ppf_log_fallback.py:349` </location>
<code_context>
+ """Test that log fallback correctly handles when beta.ppf returns an array."""
+ mini, mode, maxi = 0, 1, 10
+
+ # Test scalar input - this used to cause "ValueError: setting an array element with a sequence"
+ q_scalar = 0.5
+ result_scalar = funcs._ppf_fallback_log_space(q_scalar, mini, mode, maxi, 4)
+
+ assert isinstance(result_scalar, np.number)
+ assert mini <= result_scalar <= maxi
+ assert np.isfinite(result_scalar)
+
+ # Test array input
</code_context>
<issue_to_address>
Consider adding a test for q values at the boundaries (0 and 1) to ensure correct clipping behavior.
Explicitly test q=0 and q=1 to verify the function returns valid, finite results without errors at these boundaries.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
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 |
PR Code Suggestions ✨No code suggestions found for the PR. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 8079cf8 in 52 seconds. Click for details.
- Reviewed
56lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft 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:44
- Draft comment:
Good fix: using results.flat[i] guarantees correct assignment for multi‐dimensional arrays. Consider adding a brief inline comment explaining this usage for future maintainability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. tests/test_ppf_log_fallback.py:325
- Draft comment:
Direct comparison on a NumPy array (e.g. 'assert mini <= result_single <= maxi') may yield ambiguous truth values. Consider using np.all(result_single >= mini) and np.all(result_single <= maxi) or converting the single‐element array to a scalar (e.g. result_single.item()). - 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.
Workflow ID: wflow_oZuUJOzIIh66fvH3
You can customize 
There was a problem hiding this comment.
Code Review
The pull request correctly fixes a bug in array dimension handling for the log-space fallback by using flat indexing. The version bump is appropriate, and the new tests improve coverage for scalar and array inputs. I've added one medium-severity comment to suggest improving the clarity of the new test's name and documentation to better reflect the nature of the fix.
| def test_log_fallback_handles_beta_ppf_array_return(self): | ||
| """Test that log fallback correctly handles when beta.ppf returns an array.""" |
There was a problem hiding this comment.
The test name and docstring could be misleading about the nature of the bug being fixed.
The root cause of the issue addressed by using results.flat[i] is the handling of multi-dimensional q arrays, not necessarily beta.ppf returning an array. When q is multi-dimensional, results is as well. The loop iterates with a flat index i, so results[i] would incorrectly reference an entire row, while results.flat[i] correctly targets a single element.
For beta.ppf to return an array in this context, alpha or beta would need to be arrays, which would likely cause the brentq call in the try block to fail before reaching the fallback logic, as brentq expects a scalar-valued function.
To improve clarity and future maintainability, I recommend renaming the test and updating its docstring to more accurately reflect the problem of handling multi-dimensional inputs. For example:
To improve clarity and future maintainability, I recommend renaming the test and updating its docstring to more accurately reflect the problem of handling multi-dimensional inputs. For example, rename test_log_fallback_handles_beta_ppf_array_return to test_log_fallback_handles_multidimensional_q and update the docstring to Test that log fallback correctly handles multi-dimensional q and preserves shapes.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6dbd6aa in 1 minute and 11 seconds. Click for details.
- Reviewed
61lines of code in2files - 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:36
- Draft comment:
Using np.ndenumerate instead of enumerate(_q.flat) correctly preserves multi-dimensional indices. This is a clear improvement for handling array dimensions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. betapert/funcs.py:44
- Draft comment:
Updating assignment from results.flat[i] to results[i] aligns with the use of np.ndenumerate and improves readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/test_ppf_log_fallback.py:341
- Draft comment:
The redundant test 'test_log_fallback_handles_beta_ppf_array_return' has been removed. Ensure that existing tests adequately cover both scalar and array outputs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_cP7gJdETvtttuSPa
You can customize
|
/review |
|
Persistent review updated to latest commit 6dbd6aa |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6277e54 in 1 minute and 28 seconds. Click for details.
- Reviewed
76lines of code in2files - Skipped
0files 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:18
- Draft comment:
Consider using Python’s logging module instead of sys.stderr.write for debug messages. This improves flexibility and allows better control over logging levels. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. betapert/funcs.py:52
- Draft comment:
Good improvement: capturing the exception as 'e' allows logging detailed error info in DEBUG mode. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/test_ppf_log_fallback.py:316
- Draft comment:
Consider using np.isscalar(result_scalar) to check for scalar output instead of isinstance(result_scalar, np.number), since built‐in floats may not always be recognized as np.number. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. tests/test_ppf_log_fallback.py:327
- Draft comment:
When testing single-element array returns, avoid chain comparisons (e.g. 'assert mini <= result_single <= maxi') since they can be ambiguous for numpy arrays. Use elementwise checks like np.all(result_single >= mini) and np.all(result_single <= maxi). - 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.
Workflow ID: wflow_bVCIdCxQC4DnzxpF
You can customize
1 participant
Summary by Sourcery
Fix array dimension handling in the log-space fallback for beta PPF, add tests to verify correct behavior for scalar and array inputs, and bump the package version.
Bug Fixes:
Build:
Tests:
Summary by CodeRabbit
Bug Fixes
Chores
Important
Fix array dimension handling in
_ppf_fallback_log_spaceand add tests for scalar and array inputs, updating package version to 0.1.8._ppf_fallback_log_spaceinfuncs.pyto correctly handle array returns frombeta.ppf.np.ndenumeratefor flat indexing in_ppf_fallback_log_space.test_ppf_log_fallback.pyto verify_ppf_fallback_log_spacehandles scalar and array inputs, preserving shapes and bounds.pyproject.tomlfrom 0.1.7 to 0.1.8.This description was created by
for 6277e54. You can customize this summary. It will automatically update as commits are pushed.