api: Always make subsampling factor symbolic #2259
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2259 +/- ##
=======================================
Coverage 86.95% 86.95%
=======================================
Files 229 229
Lines 41939 41970 +31
Branches 7748 7752 +4
=======================================
+ Hits 36466 36495 +29
- Misses 4836 4838 +2
Partials 637 637
|
mloubout
force-pushed
the
symbolic-factpr
branch
from
7f869da to
6c62e3a
Compare
| @@ -793,8 +793,8 @@ def _arg_defaults(self, alias=None): | |||
| args = ReducerMap({key.name: self._data_buffer}) | |||
|
|
|||
| # Collect default dimension arguments from all indices | |||
| for i, s in zip(self.dimensions, self.shape): | |||
| args.update(i._arg_defaults(_min=0, size=s)) | |||
| for a, i, s in zip(key.dimensions, self.dimensions, self.shape): | |||
There was a problem hiding this comment.
@FabioLuporini quite surprised this never broke anything
There was a problem hiding this comment.
I'm now confused.
Shouldn't it be key._data_buffer?
Shouldn't it be key.shape?
Might just be early morning sleepy brain
There was a problem hiding this comment.
No:
key/aliasis the object and names used to build the operator, i.e the "key" of key-value pairsselfis the runtime argument with the runtime shapes/values/data/....
So you want to return the {key: self} key-pair values
There was a problem hiding this comment.
ah yes you're right of course
| @@ -793,8 +793,8 @@ def _arg_defaults(self, alias=None): | |||
| args = ReducerMap({key.name: self._data_buffer}) | |||
|
|
|||
| # Collect default dimension arguments from all indices | |||
| for i, s in zip(self.dimensions, self.shape): | |||
| args.update(i._arg_defaults(_min=0, size=s)) | |||
| for a, i, s in zip(key.dimensions, self.dimensions, self.shape): | |||
There was a problem hiding this comment.
I'm now confused.
Shouldn't it be key._data_buffer?
Shouldn't it be key.shape?
Might just be early morning sleepy brain
| @@ -890,7 +900,7 @@ def _arg_defaults(self, _min=None, size=None, alias=None): | |||
| return defaults | |||
| try: | |||
| # Is it a symbolic factor? | |||
| factor = defaults[dim._factor.name] = dim._factor.data | |||
| factor = defaults[dim._factor.name] = self._factor.data | |||
There was a problem hiding this comment.
same as above, self is the runtime argument with the value for the operator, dim is the name of the dimension used to build the operator and argument name
| @@ -848,13 +849,22 @@ def __init_finalize__(self, name, parent=None, factor=None, condition=None, | |||
|
|
|||
| super().__init_finalize__(name, parent) | |||
|
|
|||
| self._factor = factor | |||
| # Always make the factor symbolic to allow overrides with different factor. | |||
There was a problem hiding this comment.
We totally sure we don't want to leave this to the user?
Probably yes for the principle of least surprise right? the underlying issue is when you pass an override with a TimeFunction with a different factor -- correct?
A symbolic factor will make the generated code a bit more verbose (thinking about when it's used in tandem with lazy streaming), but yeah, it sounds safer...
There was a problem hiding this comment.
I think it's safer to make it symbolic. It makes it a little it more verbose yes but at the price of avoiding bad surprises.
FabioLuporini
changed the title
| self._factor = None | ||
| elif is_integer(factor): | ||
| self._factor = Constant(name="%sf" % name, value=factor, dtype=np.int32) | ||
| elif factor.is_Constant and is_integer(factor.data): |
There was a problem hiding this comment.
issubclass(factor, np.integer)
slightly cleaner
| @@ -793,8 +793,8 @@ def _arg_defaults(self, alias=None): | |||
| args = ReducerMap({key.name: self._data_buffer}) | |||
|
|
|||
| # Collect default dimension arguments from all indices | |||
| for i, s in zip(self.dimensions, self.shape): | |||
| args.update(i._arg_defaults(_min=0, size=s)) | |||
| for a, i, s in zip(key.dimensions, self.dimensions, self.shape): | |||
There was a problem hiding this comment.
ah yes you're right of course
Fix issue with overwrties with different factor