Expression mappers: Remove recurse_to_parent option and recurse by default #140
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/140/index.html |
reuterbal
force-pushed
the
133-expression-finder-retrieval-function
branch
from
0cad410
to
da58a0a
Compare
Codecov Report
@@ Coverage Diff @@
## main #140 +/- ##
=======================================
Coverage 92.10% 92.10%
=======================================
Files 89 89
Lines 16512 16501 -11
=======================================
- Hits 15208 15198 -10
+ Misses 1304 1303 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loki/expression/expr_visitors.py
Outdated
| self._retriever = ExpressionRetriever(lambda e: isinstance(e, Expression), | ||
| recurse_to_parent=recurse_to_parent) | ||
| def __init__(self, **kwargs): | ||
| self._retriever = ExpressionRetriever(lambda e: isinstance(e, Expression)) |
There was a problem hiding this comment.
This still makes the behaviour object-bound, instead of class-bound and the fact that this is static is applied post-hoc. I wonder it would be easier instead to just set a static ExpressionRetriever (expression visitor) via a class method and change the ExpressionFinder (IR visitor) to always call self.retriever.retrieve(). So for the sub-classes we would then merely do
class FindTypedSymbols(ExpressionFinder):
retriever = ExpressionRetriever(lambda x: ...)
without ever touching the __init__ of the derived instance?
reuterbal
force-pushed
the
133-expression-finder-retrieval-function
branch
from
da58a0a
to
c4d3f5f
Compare
reuterbal
force-pushed
the
133-expression-finder-retrieval-function
branch
from
c4d3f5f
to
7a1b195
Compare
|
Thanks for the comment, I had overlooked that. I have changed the implementation as suggested, making the retriever a class property. I took the opportunity to nuke the I think this is ready for another look. |
| Custom expression finder that that yields all literals of the types | ||
| specified in the config and stops recursion on loop ranges and array subscripts | ||
| (to avoid warnings about integer constants in these cases) | ||
| """ |
There was a problem hiding this comment.
[no action] Agree on this looking much nicer! 👍
| Generic visitor method that will call the :any:`ExpressionRetriever` | ||
| only on :class:`pymbolic.primitives.Expression` children, collecting | ||
| ``(node, expression root, comparison)`` tuples for all matches. | ||
| """ |
There was a problem hiding this comment.
[no action] Uhhh, nice, although quite advanced interface usage! Still, the level of documentation makes it at least accessible.
This fixes the symptom described in #133.
The retrieval function for IR visitors that recurse into expression trees is defined as a static method on the class of each such visitor. While the current way how this is being installed is debatable, it is conceptually correct. However, a later addition allowed to parameterize this retrieval function by providing the option to not recurse to "parents" of expression nodes (the
recurse_to_parentoption). This causes problems, because multiple instances of the same class may then use different variations and consequently change the behaviour of existing objects.This option relates to the used of derived type member variables, where e.g., a lookup of symbols would then yield, say
a%b%c,a%banda- but return onlya%b%cif that recursion is switched off.The option has only been switched off in very few places and there wasn't a hard dependency on it's functionality (only true use case was in the derived type flattening where the filtering can be done post-visitor as well). Therefore, I removed the option and made recursion the default behaviour.