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

fix[cartesian]: Fix minimal k-range computation #1842

Merged
merged 11 commits into from
Feb 27, 2025

Conversation

twicki
Copy link
Contributor

@twicki twicki commented Jan 31, 2025

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:

  • intervals [START+X ,END+y) are now also considered:
    • interval(3,-1) requires a minimal size of 5 for the interval to not be empty
    • interval(3,None) now requires a minimal size of 4
  • intervals [START+X, START+Y) and [END+X,END+Y) are not affected.
  • empty intervals are still allowed to have a 0-domain as to accomodate 2-dimensional fields
    • interval(...) still requires no k-size

Requirements

  • All fixes and/or new features come with corresponding tests.

Copy link
Contributor

@romanc romanc left a 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?

@twicki
Copy link
Contributor Author

twicki commented Feb 7, 2025

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?

Sor for the second stencil to make sense, the domain in k need to have size 2. Otherwise the second interval would be empty.
Yes, the extent in which computation happens is only of size 1, but these checks are here to understand if the arguments to the call are compatible with the stencil definition, and this is why we need to check domain- and field-sizes

Copy link
Contributor

@romanc romanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I understand the meaning of min_k_size now and this looks good from what I understand.

I don't understand the k_bounds thingy to be honest, hence the question if the docstring was wrong to begin with. Anyways, since we don't have changes there I guess that's okay for now and I don't need to understand.

# vertical domain size >= 16 required for test_large_k_interval
stencil(**args, origin=(10, 10, 5), domain=(3, 3, 16))
# vertical domain size > 16 required for test_large_k_interval
stencil(**args, origin=(10, 10, 5), domain=(3, 3, 17))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: also change the comment in

with interval(6, -10): # this stage will only run if field has more than 16 elements

I think it's technically still valid because this interval demands a size of 16 and the extra one comes from the interval(-10, None) below. I just - I don't like the wording because - to me - "will only run" sounds more like it will be skipped rather than throwing an error.

@romanc
Copy link
Contributor

romanc commented Feb 26, 2025

all good from my side

@FlorianDeconinck
Copy link
Contributor

cscs-ci run

@FlorianDeconinck
Copy link
Contributor

cscs-ci run

@FlorianDeconinck
Copy link
Contributor

cscs-ci run

@FlorianDeconinck
Copy link
Contributor

Gitlab ci timed out on SLURM - re-launch

@FlorianDeconinck FlorianDeconinck merged commit 70569bc into GridTools:main Feb 27, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants