Skip to content

State features: fill and data dimensions support.#250

Merged
FlorianDeconinck merged 4 commits into
NOAA-GFDL:developfrom
FlorianDeconinck:feature/state_fill_and_ddim
Oct 10, 2025
Merged

State features: fill and data dimensions support.#250
FlorianDeconinck merged 4 commits into
NOAA-GFDL:developfrom
FlorianDeconinck:feature/state_fill_and_ddim

Conversation

@FlorianDeconinck
Copy link
Copy Markdown
Collaborator

Continuing the feature set for State as we met them.

  • Introduce a fill method that will fill all fields with a particular value
  • Allow for field to be used easily with data dimensions via a temporary Sizer swap

Side task:

  • Clean up a global_config that isn't used anymore.

@FlorianDeconinck
Copy link
Copy Markdown
Collaborator Author

FlorianDeconinck commented Oct 6, 2025

Note for reviewer:

The sizer swap is an inelegant solution to a mismatch in QuantityFactory design and real use.

By design QuantityFactory is a self-contained factory that requires a precise description of the system (e.g. Sizer, dims to allocate). But the use of it is that most of the time you have a firm "Grid" definition with the cartesian space well defined, and data dimensions wildly changing per code. You could, in theory, define a QuantityFactory with every single dimensions and then call it with the one that make sense. You can't force this level of coherency on a model. Hence the mix: swap the sizer on call, when you know, and reuse the Grid space.

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.

Can we move the unrelated changes to global_config.py out into a separate PR?

Regarding the ddims swap on the sizer: I think it's quite elegant since (from the front) nobody sees that we are doing behind the scenes. Have you considered making data_dimensions completely optional, e.g.

@classmethod
def empty(
    cls,
    quantity_factory: QuantityFactory,
    *,
    data_dimensions: dict[str, int] | None = None,
) -> Self:
    """Allocate all quantities. Do not expect 0 on values, values are random.

    Args:
        quantity_factory: Quantities are allocated with this factory.
        data_dimensions: If provided, overrides the data dimensions of
            `quantity_factory`. Dict of name/size pairs or `None` (default).
    """

    if data_dimensions is None:
        return cls._init(quantity_factory.empty)

    with State._FactorySwapDimensionsDefinitions(quantity_factory, data_dimensions):
        return cls._init(quantity_factory.empty)

In that case, I'd suggest to force data_dimensions as a keyword argument. We could also think about renaming the argument to something like tmp_ddims, ddims_override, ...

Comment thread ndsl/quantity/state.py Outdated
Comment thread ndsl/quantity/state.py Outdated
@romanc
Copy link
Copy Markdown
Collaborator

romanc commented Oct 7, 2025

Ah and I forgot to mention: would a full (as in numpy) be useful to allocation and initialize to any given number (not just 0 and 1)?

Edit: Reminds me of issue #166.

Copy link
Copy Markdown
Contributor

@fmalatino fmalatino left a comment

Choose a reason for hiding this comment

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

Approve pending requested changes by other reviewers.

@FlorianDeconinck
Copy link
Copy Markdown
Collaborator Author

  • Added full - just on State, will do Quantity in another PR
  • Re: naming of data dimensions parameter

I am eager to keep data_dimensions as a name to describe the use rather than something that would hint at the implementation. Users tend to not really concern themselves with how data dimensions are carried by quantity factory, I'd rather keep straighforward. If it's confusing people, let's change

`state.full` method
+ utest update
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.

🚀

@romanc romanc mentioned this pull request Oct 8, 2025
7 tasks
@FlorianDeconinck FlorianDeconinck added this pull request to the merge queue Oct 10, 2025
Merged via the queue into NOAA-GFDL:develop with commit 2fb1393 Oct 10, 2025
5 of 6 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.

4 participants