Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: Fix ConditionalDimensions on SubDomains #2357

Closed
wants to merge 1 commit into from

Conversation

EdCaunt
Copy link
Contributor

@EdCaunt EdCaunt commented Apr 17, 2024

Tweak dimension extraction from conditions so that ConditionalDimensions can have SubDimension parents.

Now you can do

class Middle(SubDomain):
    name = 'middle'
    def define(self, dimensions):
        return {x: x, y: ('middle', 2, 4)}
mid = Middle() 
my_grid = Grid(shape = shape, subdomains = (mid, ))

_, yi = mid.dimensions
condition = Lt(sdf, 1).subs(y, yi)
ci = ConditionalDimension(name='ci', condition=condition, parent=yi)

and the ConditionalDimension will be nested inside the SubDimension in the generated loops.

@mloubout
Copy link
Contributor

So you still needs the subs eventhough the parent is yi ?

condition = Lt(sdf.subs(y, yi), 1) would maybe make more sense.

Is there a test/example for it?

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.44%. Comparing base (42cce7e) to head (5fac196).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2357   +/-   ##
=======================================
  Coverage   79.44%   79.44%           
=======================================
  Files         232      232           
  Lines       43607    43607           
  Branches     8072     8072           
=======================================
  Hits        34643    34643           
  Misses       8208     8208           
  Partials      756      756           

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

@mloubout mloubout added API api (symbolics, types, ...) compiler and removed API api (symbolics, types, ...) labels Apr 17, 2024
@EdCaunt
Copy link
Contributor Author

EdCaunt commented Apr 17, 2024

So you still needs the subs eventhough the parent is yi ?

condition = Lt(sdf.subs(y, yi), 1) would maybe make more sense.

Is there a test/example for it?

Yeah, you still need the .subs. A test needs to be added, it would probably also be a good idea to add an example

@EdCaunt
Copy link
Contributor Author

EdCaunt commented Apr 18, 2024

Closed as replicates a fix already in #2050

@EdCaunt EdCaunt deleted the conditional_subdomain branch July 9, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants