get_scalar_constant_value now raises for non-scalar inputs
#248
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ricardoV94
reviewed
Mar 17, 2023
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #248 +/- ##
==========================================
- Coverage 80.44% 80.44% -0.01%
==========================================
Files 170 170
Lines 45328 45333 +5
Branches 11069 11071 +2
==========================================
+ Hits 36463 36467 +4
Misses 6642 6642
- Partials 2223 2224 +1
|
ricardoV94
requested changes
Mar 20, 2023
pytensor/tensor/basic.py
Outdated
Comment on lines
268
to
272
| if isinstance(v, np.ndarray): | ||
| data = v.data | ||
| if data.ndim != 0: | ||
| raise NotScalarConstantError() |
There was a problem hiding this comment.
This should work with PyTensor variables. This might work
Suggested change
| if isinstance(v, np.ndarray): | |
| data = v.data | |
| if data.ndim != 0: | |
| raise NotScalarConstantError() | |
| if isinstance(v, (Variable, np.ndarray)): | |
| if v.ndim != 0: | |
| raise NotScalarConstantError() |
Please add a check testing this functionality with a PyTensor variable input. That's actually the most common case, not NumPy arrays.
There was a problem hiding this comment.
Understood, will add it right away!
pytensor/tensor/basic.py
Outdated
| @@ -255,7 +255,26 @@ def _obj_is_wrappable_as_tensor(x): | |||
| ) | |||
|
|
|||
|
|
|||
| def get_scalar_constant_value( | |||
| def get_scalar_constant(v, elemwise=True, only_process_constants=False, max_recur=10): | |||
There was a problem hiding this comment.
We shouldn't change the name, even if it's a bit verbose.
Suggested change
| def get_scalar_constant(v, elemwise=True, only_process_constants=False, max_recur=10): | |
| def get_scalar_constant_value(v, elemwise=True, only_process_constants=False, max_recur=10): |
Needs to be updated everywhere
pytensor/tensor/basic.py
Outdated
| ) | ||
|
|
||
|
|
||
| def get_underlying_scalar_constant( |
There was a problem hiding this comment.
To match the non-underlying
Suggested change
| def get_underlying_scalar_constant( | |
| def get_underlying_scalar_constant_value( |
Needs to be updated everywhere
ricardoV94
changed the title
get_scalar_constant_value now raises for non-scalar typed inputs. get_underlying_scalar_constant_value can be used for the old behavior
ricardoV94
changed the title
get_scalar_constant_value now raises for non-scalar typed inputs. get_underlying_scalar_constant_value can be used for the old behaviorget_scalar_constant_value now raises for non-scalar inputs. get_underlying_scalar_constant_value can be used for the old behavior
ricardoV94
changed the title
get_scalar_constant_value now raises for non-scalar inputs. get_underlying_scalar_constant_value can be used for the old behaviorget_scalar_constant_value now raises for non-scalar inputs
ricardoV94
added
enhancement
shreyas3156
force-pushed
the
raise-non-scalar-226
branch
from
303094f
to
1981bb5
Compare
|
@ricardoV94 It seems like the benchmark test is failing for a couple of tests in |
|
That's probably just a fluke, nothing to worry about |
ricardoV94
approved these changes
Mar 22, 2023
|
Thanks @shreyas3156 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for these changes
Closes #226
Implementation details
get_scalar_constant()method that raises NotScalarConstantError when non-scalar outputs are passed.get_scalar_constant_value()toget_underlying_scalar_constant()Checklist
Major / Breaking Changes
Bugfixes