Skip to content
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

feat(jax): Allow boolean masks with constants #376

Closed
wants to merge 1 commit into from

Conversation

aseyboldt
Copy link
Member

@aseyboldt aseyboldt commented Jul 12, 2023

Indexing with boolean arrays is problematic in jax, because it always needs to know the shape of the result. But this is the case if the mask is a constant, so we can allow that.

TODO needs a test...

@codecov-commenter
Copy link

Codecov Report

Merging #376 (2218dbe) into main (981be2a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #376   +/-   ##
=======================================
  Coverage   80.43%   80.44%           
=======================================
  Files         156      156           
  Lines       45447    45472   +25     
  Branches    11125    11137   +12     
=======================================
+ Hits        36555    36579   +24     
  Misses       6687     6687           
- Partials     2205     2206    +1     
Impacted Files Coverage Δ
pytensor/link/jax/dispatch/subtensor.py 78.68% <100.00%> (-0.98%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

You might need to inline it in the dispatched function, something like what we do for here

if isinstance(shape, Constant):

@ricardoV94
Copy link
Member

I think this was addressed by #849

@ricardoV94 ricardoV94 closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants