Skip to content

Allow passing static shape to tensor creation helpers #118

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
merged 2 commits into from
Dec 14, 2022

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Dec 13, 2022

Closes #61

Major / Breaking Changes

  • pytensor.tensor.tensor now accepts name as the only positional argument. All other arguments must be provided via keyword

New features

  • Tensor creator helpers now accept static input shapes (e.g., pt.matrix(shape=(10, None)))

@ricardoV94 ricardoV94 added the enhancement New feature or request label Dec 13, 2022
@ricardoV94 ricardoV94 force-pushed the improve_tensor_helpers branch from c58aef3 to 9e714a6 Compare December 13, 2022 12:20
* Also default dtype to "floatX" when using `tensor`
@ricardoV94 ricardoV94 force-pushed the improve_tensor_helpers branch from 9e714a6 to c5bfbcd Compare December 13, 2022 13:11
Copy link
Member

@ferrine ferrine left a comment

Choose a reason for hiding this comment

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

Happy with the changes!

@ricardoV94 ricardoV94 force-pushed the improve_tensor_helpers branch 2 times, most recently from dc340ce to 18c3cc3 Compare December 14, 2022 10:03
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

I found one thing in a test

generally I'm worried that the breaking change to require explicit kwargs will cause a lot of things to break downstream.

Requiring shape to be an explicit kwarg (but not dtype) should be much smoother, no?

@ricardoV94
Copy link
Member Author

ricardoV94 commented Dec 14, 2022

Requiring shape to be an explicit kwarg (but not dtype) should be much smoother, no?

The reason for that was that tensor behaved differently. tensor accepted dtype as the first positional argument, and name as kwarg only, whereas all other constructors accepted name as the first positional argument, followed by dtype.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Dec 14, 2022

For context, I was going just for the first commit in this PR, which is backwards compatible, and @ferrine asked we made tensor have the same API as the rest.

@ricardoV94 ricardoV94 force-pushed the improve_tensor_helpers branch 2 times, most recently from ba0278b to f6fdfd6 Compare December 14, 2022 10:58
* Name is now the only optional non-keyword argument for all constructors
@ricardoV94 ricardoV94 force-pushed the improve_tensor_helpers branch from f6fdfd6 to 7b621c4 Compare December 14, 2022 11:00
@ferrine
Copy link
Member

ferrine commented Dec 14, 2022

Yes, I believe uniform API is much better long term, and since we anyway just made the fork, we better make these changes sooner than later

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Okay, I just checked the PyMC codebase and found no places where it would actually break.

@codecov-commenter
Copy link

Codecov Report

Merging #118 (7b621c4) into main (16d1cbe) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   74.41%   74.46%   +0.04%     
==========================================
  Files         179      179              
  Lines       49249    49282      +33     
  Branches    10422    10432      +10     
==========================================
+ Hits        36649    36696      +47     
+ Misses      10295    10282      -13     
+ Partials     2305     2304       -1     
Impacted Files Coverage Δ
pytensor/sparse/basic.py 82.53% <ø> (ø)
pytensor/sparse/rewriting.py 75.51% <ø> (ø)
pytensor/tensor/basic.py 89.95% <100.00%> (ø)
pytensor/tensor/blas.py 79.67% <100.00%> (ø)
pytensor/tensor/io.py 78.15% <100.00%> (ø)
pytensor/tensor/math.py 90.40% <100.00%> (ø)
pytensor/tensor/shape.py 91.74% <100.00%> (ø)
pytensor/tensor/type.py 94.19% <100.00%> (+3.24%) ⬆️
pytensor/graph/basic.py 88.13% <0.00%> (+0.03%) ⬆️
pytensor/scalar/basic.py 79.10% <0.00%> (+0.08%) ⬆️
... and 2 more

@ferrine ferrine merged commit 5c63ee7 into pymc-devs:main Dec 14, 2022
@ricardoV94 ricardoV94 deleted the improve_tensor_helpers branch January 20, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow shape specification in tensor constructor helpers
4 participants