-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix[cartesian]: Fix minimal k-range computation #1842
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused. In my mental model, min_k_size
is the minimal height of the vertical column. Why do
def stencil_no_extent_1(field_a: gs.Field[float], field_b: gs.Field[float]):
with computation(PARALLEL), interval(0, 2):
field_a = field_b[0, 0, 0]
def stencil_no_extent_2(field_a: gs.Field[float], field_b: gs.Field[float]):
with computation(PARALLEL), interval(1, 2):
field_a = field_b[0, 0, 0]
both have a min_k_size
of 2? Both have the same upper bound of 2, but the second one starts at the first level. I'd thus expect 2 for the first one and 1 for the second one. This was already before your changes, so maybe I don't understand correctly. What's wrong with my thinking?
@@ -70,17 +76,30 @@ def compute_k_boundary( | |||
return KBoundaryVisitor().visit(node, include_center_interval=include_center_interval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like include_center_interval
is also never explicitly set in this function/visitor and its call tree. Remove it too for consistency?
Also, I can't mark the KBoundaryVisitor
's doc string, but I think it should change too, no?
- """For every field compute the boundary in k, e.g. (2, -1) if [k_origin-2, k_origin+k_domain-1] is accessed."""
+ """For every field compute the boundary in k, e.g. (2, -1) if [k_origin+2, k_origin+k_domain-1] is accessed."""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: docstring:
Nothing changes on this visitor, it still seems to hold up
Sor for the second stencil to make sense, the domain in k need to have size 2. Otherwise the second interval would be empty. |
Description
The computation of the minimal k-ranges that happen during the vaildate-args step is allowing for inconsistent computation to happen. This PR stiffens the requirements on fields:
[START+X ,END+y)
are now also considered:interval(3,-1)
requires a minimal size of 5 for the interval to not be emptyinterval(3,None)
now requires a minimal size of 4[START+X, START+Y)
and[END+X,END+Y)
are not affected.interval(...)
still requires no k-sizeRequirements