feat[next]: concat_where for boundary conditions #1468
Conversation
… into concat_where
| mask: core_defs.NDArrayObject, | ||
| ) -> list[tuple[bool, common.UnitRange]]: | ||
| """Take a 1-dimensional mask and return a sequence of mappings from boolean values to ranges.""" | ||
| # TODO: does it make sense to upgrade this naive algorithm to numpy? |
There was a problem hiding this comment.
if used in k, probably not relevant, if used in the horizontal, probably this is a performance problem
There was a problem hiding this comment.
Maybe some timeit microbenchmarks with typical (both small and large) domain sizes would help to figure out if it'd pay off.
| @@ -97,14 +97,57 @@ def _absolute_sub_domain( | |||
| return common.Domain(*named_ranges) | |||
|
|
|||
|
|
|||
| def intersect_domains(*domains: common.Domain) -> common.Domain: | |||
| def domain_intersection( | |||
There was a problem hiding this comment.
Just a suggestion, I think this could be transformed into the .intersection() method in the Domain class to follow the same API as set (https://devdocs.io/python~3.12/library/stdtypes#set)
There was a problem hiding this comment.
maybe adding that makes sense, however I like more that this function works also when domains is empty
src/gt4py/next/embedded/common.py
Outdated
| return functools.reduce( | ||
| operator.and_, | ||
| domains, | ||
| common.Domain(dims=tuple(), ranges=tuple()), | ||
| ) | ||
|
|
||
|
|
||
| def intersect_domains( |
There was a problem hiding this comment.
I find this function confusing specially given the previous one. I think the name could contain the name restrict instead of intersection (e.g. restrict_to_lower_bound, restrict_to_intersection) and the ignore_dims argument shouldn't be optional, because otherwise is just a simple intersection of domains.
There was a problem hiding this comment.
- Changed the name.
- In generic context allowing ignore_dims to be optional makes sense to me. Maybe I could change the API to only accept tuples and default to empty tuple. What do you think?
There was a problem hiding this comment.
I still don't like that much that ignore_dims is optional but it's ok. ignore_dims accepting only tuples makes sense to me, but it's not strictly needed and it's orthogonal to my original complaint because it'll still have a default value.
| mask: core_defs.NDArrayObject, | ||
| ) -> list[tuple[bool, common.UnitRange]]: | ||
| """Take a 1-dimensional mask and return a sequence of mappings from boolean values to ranges.""" | ||
| # TODO: does it make sense to upgrade this naive algorithm to numpy? |
There was a problem hiding this comment.
Maybe some timeit microbenchmarks with typical (both small and large) domain sizes would help to figure out if it'd pay off.
Introduces `concat_where` to be used for boundary conditions. `where` will intersect all 3 fields. Therefore `where(klevel == 0, boundary_layer, interior)` will not work if `boundary_layer` is only defined on `klevel == 0`. `concat_where` will concatenate the fields that are selected by the mask regions. The `mask` is currently required to be 1 dimensional.
Introduces
concat_whereto be used for boundary conditions.wherewill intersect all 3 fields. Thereforewhere(klevel == 0, boundary_layer, interior)will not work ifboundary_layeris only defined onklevel == 0.concat_wherewill concatenate the fields that are selected by the mask regions. Themaskis currently required to be 1 dimensional.