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

Allow broadcasting in Elemwise.c_code #928

Merged
merged 1 commit into from
May 6, 2022

Conversation

ricardoV94
Copy link
Contributor

I think this removes any necessity for unbroadcast uses mentioned in #915 and #916.

Closes #335

@ricardoV94 ricardoV94 force-pushed the broadcast_elemwise_c_code branch 2 times, most recently from 1b2c08b to 6c1c49f Compare April 25, 2022 12:55
@ricardoV94 ricardoV94 marked this pull request as draft April 25, 2022 13:13
@ricardoV94 ricardoV94 force-pushed the broadcast_elemwise_c_code branch 2 times, most recently from b3f6c8a to 8cbe18e Compare April 25, 2022 13:59
@ricardoV94 ricardoV94 force-pushed the broadcast_elemwise_c_code branch 2 times, most recently from d3b3d5c to ba58923 Compare April 25, 2022 14:15
@ricardoV94 ricardoV94 marked this pull request as ready for review April 25, 2022 15:19
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #928 (aaa9c3b) into main (b60cf72) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #928      +/-   ##
==========================================
- Coverage   78.92%   78.92%   -0.01%     
==========================================
  Files         152      152              
  Lines       47701    47713      +12     
  Branches    10862    10862              
==========================================
+ Hits        37649    37658       +9     
- Misses       7550     7551       +1     
- Partials     2502     2504       +2     
Impacted Files Coverage Δ
aesara/scalar/basic.py 79.05% <ø> (ø)
aesara/tensor/elemwise.py 88.17% <100.00%> (-0.27%) ⬇️
aesara/tensor/elemwise_cgen.py 95.74% <100.00%> (+0.29%) ⬆️
aesara/compile/function/types.py 79.91% <0.00%> (-0.14%) ⬇️

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Just some minor updates; otherwise, I don't see anything obviously amiss, so we might be able to merge this and simply be ready to fix and/or revert if anything comes up.

aesara/tensor/elemwise_cgen.py Outdated Show resolved Hide resolved
aesara/tensor/elemwise_cgen.py Outdated Show resolved Hide resolved
aesara/tensor/elemwise_cgen.py Outdated Show resolved Hide resolved
aesara/tensor/elemwise_cgen.py Outdated Show resolved Hide resolved
tests/tensor/test_elemwise.py Outdated Show resolved Hide resolved
@brandonwillard brandonwillard changed the title Allow broadcasting in Elemwise c_code Allow broadcasting in Elemwise.c_code Apr 29, 2022
@brandonwillard
Copy link
Member

After looking at this and working on the c_code implementations for #657, I was brought back to this thread, because I can't help thinking about how simple this would all be if we could just use Cython-generated C code for these c_code implementations.

If you're feeling adventurous, give that thread a read and see if you can construct a _CThunk that uses a Cython-generated extension module. Last I recall, it seemed very doable, so, if it's something that could be hacked together in a few hours, it might be extremely helpful for the (hopefully short) time we'll be spending maintaining this annoying string-based C code.

aesara/tensor/elemwise.py Outdated Show resolved Hide resolved
aesara/tensor/elemwise.py Outdated Show resolved Hide resolved
aesara/tensor/elemwise_cgen.py Outdated Show resolved Hide resolved
aesara/tensor/elemwise_cgen.py Outdated Show resolved Hide resolved
@ricardoV94
Copy link
Contributor Author

After looking at this and working on the c_code implementations for #657, I was brought back to this thread, because I can't help thinking about how simple this would all be if we could just use Cython-generated C code for these c_code implementations.

If you're feeling adventurous, give that thread a read and see if you can construct a _CThunk that uses a Cython-generated extension module. Last I recall, it seemed very doable, so, if it's something that could be hacked together in a few hours, it might be extremely helpful for the (hopefully short) time we'll be spending maintaining this annoying string-based C code.

How would Cython make our lives easier? Mainly to avoid having to write code as Python strings?

@brandonwillard
Copy link
Member

brandonwillard commented May 5, 2022

How would Cython make our lives easier? Mainly to avoid having to write code as Python strings?

It helps with correctness as well, since Cython will take care of all the reference counting.

Also, some (or potentially a lot) of the C code we have implemented as Python strings is already implemented via Cython. For instance, x + y and/or np.add(x, y) in Cython will transpile to the NumPy C API calls that we're currently trying to emulate.

This removes an inconsistency between Numpy and Aesara broadcasting rules, where a variable dimension with unknown shape was always assumed to be non-broadcastable (i.e., different than 1)
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

All right, let's do this.

@ricardoV94 ricardoV94 merged commit c7b416e into aesara-devs:main May 6, 2022
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.

C Elemwise implementation doesn't broadcast variables
2 participants