Made splitting only happen at densification. #30
Conversation
|
Warning Rate limit exceeded@mmcdermott has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 3 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in the pull request focus on simplifying the handling of nested ragged tensors in the Changes
Assessment against linked issues
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 (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/nested_ragged_tensors/ragged_numpy.py (11 hunks)
Additional context used
Ruff
src/nested_ragged_tensors/ragged_numpy.py
555-558: Use ternary operator
vals_end = offset if len(B) == 0 else B[-1] + offsetinstead ofif-else-blockReplace
if-else-block withvals_end = offset if len(B) == 0 else B[-1] + offset(SIM108)
Additional comments not posted (6)
src/nested_ragged_tensors/ragged_numpy.py (6)
90-90: LGTM!The changes simplify the initialization of tensors by flattening nested lists into a single array, which reduces complexity in the data structure and allows for a more straightforward inference of data types. The changes are consistent with the summary provided.
255-255: LGTM!The changes simplify the handling of nested ragged tensors during initialization. Flattening the values before processing reduces complexity and the need for intermediate data structures. The changes are consistent with the summary provided.
Also applies to: 257-257, 271-271
320-321: LGTM!The changes simplify the loading of tensors by removing unnecessary checks for meta keys, which leads to a more direct assignment of tensor values. The changes are consistent with the summary provided.
525-525: LGTM!The changes simplify the logic for determining start and end indices during slicing operations. The bounds are correctly applied without redundant calculations, improving efficiency. The changes also enhance the clarity of the slicing logic. The changes are consistent with the summary provided.
Also applies to: 550-559, 560-564
983-991: LGTM!The changes improve the error handling during concatenation operations. Clearer feedback is provided when concatenation fails, making it easier to diagnose issues. The changes are consistent with the summary provided.
1105-1105: LGTM!The changes simplify the
load_slicemethod by removing the split operation. The values are directly assigned instead, reducing unnecessary processing. The changes are consistent with the summary provided.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
- Coverage 92.77% 92.49% -0.28%
==========================================
Files 2 2
Lines 332 333 +1
==========================================
Hits 308 308
- Misses 24 25 +1 ☔ View full report in Codecov by Sentry. |
|
This currently causes a significant reduction in overall speed, and, unexpectedly (and unimportantly) a degradation in preparation time: |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
.github/workflows/benchmark.yaml (2)
38-38: Monitor the impact of the lower alert threshold.Lowering the
alert-thresholdto "150%" can help detect performance regressions more quickly. However, it may also increase the number of false positives if the performance fluctuates due to factors unrelated to the code changes. Consider monitoring the impact of this change and adjusting the threshold if needed to strike a balance between early detection and reducing false positives.
40-40: Implement a process to review and address performance alerts.Setting
fail-on-alerttofalseallows the job to complete even if performance regressions are detected, enabling further analysis or manual intervention. However, it's important to implement a process to review and address the alerts generated by the job to ensure that performance regressions are not overlooked. Consider establishing a workflow to regularly monitor the alerts, investigate the causes, and take appropriate actions to maintain the desired level of performance.
This reverts commit b5f3c44. This change, despite using more numpy specific logic, had major negative consequences on overall performance.
|
Skipping the subdivision into lists and also not using |
Closes #21
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores