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

Clean up Scan's Cython implementation #774

Merged
merged 9 commits into from
Jan 23, 2022

Conversation

brandonwillard
Copy link
Member

@brandonwillard brandonwillard commented Jan 21, 2022

This PR moves all of the non-native Python, non-NumPy objects out of Scan's Cython implementation so that it has fewer interactions with Python and can—potentially—be turned into a pure C function (i.e. cdef) without too much effort.

Currently, the only function argument that isn't a native or NumPy object is the thunk for the inner-function. When the thunk is a CThunk, we can obtain a Capsule object that I believe Cython cdef functions can use directly; however, that has not been implemented in this PR.

Aside from these simple refactorings, both the Python and Cython implementations have been changed so that they use np.empty to allocate memory. That should provide a small improvement over the current use of np.zeros.

@brandonwillard brandonwillard force-pushed the more-scan-refactoring branch 2 times, most recently from 024b29e to 6da114f Compare January 21, 2022 03:52
@brandonwillard brandonwillard self-assigned this Jan 21, 2022
@brandonwillard brandonwillard added enhancement New feature or request refactor This issue involves refactoring Scan Involves the `Scan` `Op` labels Jan 21, 2022
@brandonwillard brandonwillard force-pushed the more-scan-refactoring branch 6 times, most recently from 6bb452b to 7841bbd Compare January 21, 2022 23:13
@brandonwillard brandonwillard force-pushed the more-scan-refactoring branch 2 times, most recently from ae3e4dc to bb2ac94 Compare January 22, 2022 02:29
@codecov
Copy link

codecov bot commented Jan 22, 2022

Codecov Report

Merging #774 (bb2ac94) into main (dee152c) will decrease coverage by 0.05%.
The diff coverage is 82.00%.

❗ Current head bb2ac94 differs from pull request most recent head fea7597. Consider uploading reports for the commit fea7597 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
- Coverage   78.40%   78.34%   -0.06%     
==========================================
  Files         152      152              
  Lines       47782    47793      +11     
  Branches    10881    10884       +3     
==========================================
- Hits        37462    37444      -18     
- Misses       7773     7797      +24     
- Partials     2547     2552       +5     
Impacted Files Coverage Δ
aesara/scan/op.py 81.79% <74.28%> (-0.60%) ⬇️
aesara/compile/function/pfunc.py 83.59% <100.00%> (+0.26%) ⬆️
aesara/scan/basic.py 86.68% <100.00%> (-0.82%) ⬇️
aesara/scan/scan_perform_ext.py 80.32% <100.00%> (ø)
aesara/scan/utils.py 87.44% <100.00%> (+0.01%) ⬆️
aesara/compile/io.py 83.01% <0.00%> (-5.67%) ⬇️
aesara/scan/opt.py 79.48% <0.00%> (-0.84%) ⬇️
aesara/printing.py 48.58% <0.00%> (-0.37%) ⬇️
aesara/tensor/basic_opt.py 85.03% <0.00%> (-0.19%) ⬇️
... and 2 more

@brandonwillard brandonwillard merged commit 77bcd68 into aesara-devs:main Jan 23, 2022
@brandonwillard brandonwillard deleted the more-scan-refactoring branch January 23, 2022 02:24
@zoj613
Copy link
Member

zoj613 commented Jan 27, 2022

i'm late but I think there is a bit more juice we can squeeze out of scan_perform.pyx as far as minimizing interaction with python objects. The obvious one is using memory views instead off array buffers since they are considered more performant and more flexible (see here). i can investigate a bit and send a PR if this is considered worthy of the effort.

@brandonwillard
Copy link
Member Author

i'm late but I think there is a bit more juice we can squeeze out of scan_perform.pyx as far as minimizing interaction with python objects. The obvious one is using memory views instead off array buffers since they are considered more performant and more flexible (see here). i can investigate a bit and send a PR if this is considered worthy of the effort.

That sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor This issue involves refactoring Scan Involves the `Scan` `Op`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Scan's compilation performance by improving its handling of inner graphs
2 participants