api: revamp subsampling factors to avoid duplicates

[mloubout]

No description provided.

[Copilot]

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):

[codecov]

Codecov Report

Attention: Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.91%. Comparing base (4196aa4) to head (f59a4fe).

Files with missing lines	Patch %	Lines
devito/types/dimension.py	97.05%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2575   +/-   ##
=======================================
  Coverage   91.91%   91.91%           
=======================================
  Files         245      245           
  Lines       48313    48362   +49     
  Branches     4244     4248    +4     
=======================================
+ Hits        44406    44454   +48     
+ Misses       3232     3231    -1     
- Partials      675      677    +2     

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	72.54% <93.18%> (+0.01%)	⬆️
pytest-gpu-nvc-nvidiaX	73.61% <93.18%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[EdCaunt]

Looks ok to me. Where were the duplicated subsampling factors becoming an issue?

[FabioLuporini]

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'

[FabioLuporini]

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

[mloubout]

as a DataSymbol still allows user overrides while not carrying any data explicitly

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"

[mloubout]

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.

[FabioLuporini]

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?

[mloubout]

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.

[FabioLuporini]

assuming it's possible to turn symbolic_factor into a cached_property, here we would need if self.factor. is not None

[EdCaunt]

Docstring needs updating since symbolic_factor now included in kwargs. How is symbolic_factor different to factor?

[mloubout]

factor is just the number it was created with, symbolic_factor is the symbol in the code but it's a plain Scalar that doesn't carry data and can be passed at apply.

[EdCaunt]

Makes sense. Does it _rebuild correctly? I seem to remember that there is still an issue where rebuilding a ConditionalDimension with no factor introduces a factor of one unless that got patched when I wasn't looking

[mloubout]

This rebuild issue been patch a while back yes

[FabioLuporini]

my last worry, after looking at this line, is that this set of changes might somehow break user code, because now .factor is a different thing, while the user may have expected a Constant if a Constant was supplied