Skip to content

Store information about the halo-size in the allocated Quantity#265

Merged
twicki merged 3 commits into
NOAA-GFDL:developfrom
twicki:pass_nhalo_to_quanity
Oct 22, 2025
Merged

Store information about the halo-size in the allocated Quantity#265
twicki merged 3 commits into
NOAA-GFDL:developfrom
twicki:pass_nhalo_to_quanity

Conversation

@twicki
Copy link
Copy Markdown
Collaborator

@twicki twicki commented Oct 15, 2025

Description

In order to have the option to check for correct sizes on data, the information of what halo-size was used to generate the Quantity should be stored with the Quantity.

This is (self-contained) preliminary work to handle #239 properly, as we've seen issues without this information.

How has this been tested?

Just CI-framework.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New check tests, if applicable, are included

Copy link
Copy Markdown
Collaborator

@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.

Looking good to me - just a question about naming things, which is - as always - the hardest part 😄 🙈

Comment thread ndsl/quantity/quantity.py Outdated
computational domain. Defaults to None.
extent (Sequence[int] | None, optional): number of points along each axis
within the computational domain. Defaults to None.
gt4py_backend (str | None, optional): _description_. Defaults to None.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is _description_ a magic docstring feature that I don't know about or did we loose the description of gt4py_backend here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

bad cleanup, thanks

Comment thread ndsl/quantity/quantity.py
gt4py_backend (str | None, optional): _description_. Defaults to None.
allow_mismatch_float_precision (bool, optional): allow for precision that is
not the simulation-wide default configuration. Defaults to False.
number_of_halo_points (int, optional): Number of halo points used. Defaults to 0.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not so sure about the name here because it's not really the number of halo points, right? If I put 3, there will be more then 3 halo points. Though not optimal either, I'd stick with what we already have (n_halo) to avoid confusion. Maybe we could do "halo size" instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

well, n_halo is just number of halo points, as per

n_halo: number of halo points

or
"""Number of horizontal halo points for produced arrays."""

or
and number of halo points.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

then let's change all of them because they are equally wrong in that sense?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is the lingo that is used to describe the halo size. This is not changeable

Copy link
Copy Markdown
Collaborator

@romanc romanc Oct 15, 2025

Choose a reason for hiding this comment

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

Isn't our job here to dare new things and sometimes break with (old) conventions? Like, we tell atmospheric scientists that they can now write code in python(-ish) and that they can have variable names longer than 5 characters?

We even make use of the term "halo size" in our introduction examples when referring to the nhalo argument of the boilerplate functions.

"The `StencilFactory` object enables the sharing of stencil properties across multiple stencils as well as \"build and execute\" the stencil. To help ease the introduction, the [`boilerplate` module](./boilerplate.py) contains a function `get_one_tile_factory` that takes the domain size, halo size, and backend of interest and returns a `StencilFactory` object. For more details about the objects needed to create the `StencilFactory`, the reader can view the [`get_one_tile_factory`](./boilerplate.py#get_one_tile_factory) function."

"Next, we'll create a simple driver that defines the domain and halo size, specifies the backend (`dace:cpu` in order to use DaCe), and uses the boilerplate code to create a stencil and quantity factory objects. These objects help define the computational domain used for this particular example. After defining quantities (`in_field` and `out_field`) to hold the appropriate values and creating an object `local_sum` for our combined stencil/Python calculation, `local_sum` is called to perform the computation. In the output, we can see DaCe orchestrating the code. "

Comment thread ndsl/quantity/quantity.py Outdated
Comment thread ndsl/quantity/metadata.py
extent: Tuple[int, ...]
"the shape of the computational domain"
n_halo: int
"Number of halo-points used in the horizontal"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe "halo size" rather than "halo points"?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

see above

@FlorianDeconinck FlorianDeconinck added High Priority Extra attention is needed Scheduled for next Release To be merged before the next release labels Oct 15, 2025
@twicki twicki added this pull request to the merge queue Oct 22, 2025
Merged via the queue into NOAA-GFDL:develop with commit f10c47d Oct 22, 2025
6 checks passed
@twicki twicki mentioned this pull request Oct 23, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

High Priority Extra attention is needed Scheduled for next Release To be merged before the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants