Skip to content

Allow broadcasting in specialized numba dispatch of AdvancedIncSubtensor #1272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Mar 6, 2025

@ricardoV94 ricardoV94 force-pushed the more_advanced_inc_subtensor_numba branch from bcb9ebd to ec81f12 Compare March 6, 2025 15:03
Copy link

codecov bot commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.00%. Comparing base (89d5366) to head (ec81f12).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/link/numba/dispatch/subtensor.py 95.34% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (95.34%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1272   +/-   ##
=======================================
  Coverage   81.99%   82.00%           
=======================================
  Files         188      188           
  Lines       48608    48617    +9     
  Branches     8688     8690    +2     
=======================================
+ Hits        39857    39866    +9     
  Misses       6586     6586           
  Partials     2165     2165           
Files with missing lines Coverage Δ
pytensor/link/numba/dispatch/subtensor.py 95.58% <95.34%> (+0.23%) ⬆️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

True,
True,
False,
False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we concoct a new test case that still requires object on both inc and set? Or is the coverage now that good?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 such tests already:

(
np.arange(3 * 4 * 5).reshape((3, 4, 5)),
rng.poisson(size=(2, 4)),
([1, 2], slice(None), [3, 4]), # Non-consecutive vector indices
False,
True,
True,
),
(
np.arange(3 * 4 * 5).reshape((3, 4, 5)),
rng.poisson(size=(2, 2)),
(
slice(1, None),
[1, 2],
[3, 4],
), # Mixed double vector index and basic index
False,
True,
True,
),

np.arange(3 * 5).reshape((3, 5)),
rng.poisson(size=(1, 2, 2)), # Same as before, but Y broadcasts
(slice(1, 3), [[1, 2], [2, 3]]),
False,
True,
True,
),

Either when array indexes are non-consecutive, or mixed with basic indices

@ricardoV94 ricardoV94 merged commit 0de0fa9 into pymc-devs:main Mar 13, 2025
72 of 73 checks passed
@ricardoV94 ricardoV94 deleted the more_advanced_inc_subtensor_numba branch March 13, 2025 13:28
@ricardoV94 ricardoV94 changed the title Allow broadcasting in specialized numba dispatch of AdvancedIncSubtensor Allow broadcasting in specialized numba dispatch of AdvancedIncSubtensor Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants