api: revamp subsampling factors to avoid duplicates - #2575
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)tests/test_dimension.py:1900
- [nitpick] Consider renaming 'test_cond_copy' to a more descriptive name (e.g., 'test_operator_parameters_caching') to better convey the intent of the test.
def test_cond_copy(self):
devito/types/dimension.py:826
- [nitpick] Consider adding a docstring to 'SubsamplingFactor' to clearly document its caching functionality and intended usage.
class SubsamplingFactor(Constant, Cached):
Sorry, something went wrong.
Codecov ReportAll modified and coverable lines are covered by tests ✅ Additional details and impacted files @@ Coverage Diff @@
## main #2575 +/- ##
=======================================
Coverage 91.91% 91.92%
=======================================
Files 245 245
Lines 48313 48363 +50
Branches 4244 4248 +4
=======================================
+ Hits 44406 44456 +50
+ Misses 3232 3231 -1
- Partials 675 676 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
EdCaunt
left a comment
There was a problem hiding this comment.
Looks ok to me. Where were the duplicated subsampling factors becoming an issue?
Sorry, something went wrong.
There was a problem hiding this comment.
What makes me think here is that it feels like we're relying on caching for correctness, and that shouldn't the case. Caching should just be a thing for performance. We know it's not always been like that in the past, but we've improved the situation over the years. This however looks like a step back
Also, it doesn't really make much sense that we use a Constant that is an Uncached, being a subclass of DataSymbol, and now here we have a subclass of Constant that is Cached instead, as it's like bending the class hierarchy.
I think the true issue here is that we're using a Constant, whose underlying value can change dynamically, as subsampling factor.
So, with all this in mind, rolling back to our starting point, I think a potentially better solution here is to switch from Constant to DataSymbol, as a DataSymbol still allows user overrides while not carrying any data explicitly, and so behaving as expected. Check the following out -- no surprises.
In [1]: from devito.types import Constant In [2]: a = Constant(name='a') In [3]: a1 = Constant(name='a1') In [4]: a == a1 Out[4]: False ... In [8]: from devito.types.basic import DataSymbol In [9]: b = DataSymbol(name='b') In [10]: b1 = DataSymbol(name='b') In [11]: b == b1 Out[11]: True In [12]: b.data --------------------------------------------------------------------------- AttributeError Traceback (most recent call last) Cell In[12], line 1 ----> 1 b.data AttributeError: 'DataSymbol' object has no attribute 'data'
Sorry, something went wrong.
|
for maximum neatness, we could still have a class SubsamplingFactor(DataSymbol): pass user-provided Constants as factors should still be supported for backwards compatibility obviously |
Sorry, something went wrong.
Well, that's the issue, it needs to carry some, forcing the user to provide the value at apply when they already provided it at the dimension creation is not good. The API is "it allows overrides" not "you have to provide it at apply" |
Sorry, something went wrong.
|
Also you need b is b1 to be True not b == b1 for the symbol to be treated as unique and since DataSymbol is Uncached it won't work or will require the same massaging to make it cached. |
Sorry, something went wrong.
|
|
||
| if self._factor is not None: | ||
| # Always make the factor symbolic to allow overrides with different factor. | ||
| self._symbolic_factor = symbolic_factor or \ |
There was a problem hiding this comment.
why does this need to be an instance attribute? just like for example symbolic_size or symbolic_max are cached_property, can (should) this not be the same?
Sorry, something went wrong.
There was a problem hiding this comment.
Need it there so that it uses the name of the input Constant when one is used for backward compatibility. Can't do that in a cached_property.
Sorry, something went wrong.
| # Parent dimension define the interval | ||
| fact = self._factor.data if self._factor is not None else 1 | ||
| def _arg_values(self, interval, grid=None, args=None, **kwargs): | ||
| if self.symbolic_factor is not None: |
There was a problem hiding this comment.
assuming it's possible to turn symbolic_factor into a cached_property, here we would need if self.factor. is not None
Sorry, something went wrong.
No description provided.