Skip to content

Field Bundle: Functional code for 4D array#143

Merged
FlorianDeconinck merged 4 commits into
NOAA-GFDL:developfrom
FlorianDeconinck:feature/field_bundle_boilerplate
May 16, 2025
Merged

Field Bundle: Functional code for 4D array#143
FlorianDeconinck merged 4 commits into
NOAA-GFDL:developfrom
FlorianDeconinck:feature/field_bundle_boilerplate

Conversation

@FlorianDeconinck
Copy link
Copy Markdown
Collaborator

@FlorianDeconinck FlorianDeconinck commented May 16, 2025

Description

  • Boilerplate code taking in a 4D Quantity
  • Name/Index indexer
  • Natively orchastreatable
  • Deferred type hint for data dimensions
  • Unit tests

How Has This Been Tested?
New units tests for basic usage

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
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included

Deferred type hint for data dimensions
Unit tests
romanc
romanc previously approved these changes May 16, 2025
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.

Proper dark magic, but that's what the DSL is here for: use whatever is needed on the inside to make it look nice on the outside.

Do I understand correctly that the current semantics are as follows:

  • FieldBundle adds a 4th dimension,
  • the 4th dimension can have any size,
  • the 4th dimension is always a float (from field_bundle.py, line 130)

While the 4th dimension is clearly stated everywhere, the Float type might surprise developers.

I do have one structural question: what happens if developers initialize a FieldBundle without a mapping? Won't that break the map() / index() (and by extension __getattr__()) functions?

Apart from this, I think structurally, this can go in as-is for a first version. When we move to more than 4 dimensions, we can revisit the details. Might be easier once we have a bit of experience with the bundles. I left a few of nitpicks inside - nothing urgent. My wishlist (for future work) includes

  • FieldBundleType: docstrings for the two methods and unit test (register, re-register, and type resolving with/without markup)
  • FieldBundle docstrings

Comment thread ndsl/quantity/field_bundle.py Outdated
Comment thread ndsl/quantity/field_bundle.py Outdated
Comment thread ndsl/quantity/field_bundle.py Outdated
Comment thread ndsl/dsl/stencil.py Outdated
Comment thread ndsl/quantity/field_bundle.py
Comment thread ndsl/quantity/field_bundle.py Outdated
Comment thread ndsl/quantity/field_bundle.py Outdated
Comment thread ndsl/quantity/field_bundle.py Outdated
Modernizing type hint
Docstrings
@FlorianDeconinck
Copy link
Copy Markdown
Collaborator Author

Proper dark magic, but that's what the DSL is here for: use whatever is needed on the inside to make it look nice on the outside.

Do I understand correctly that the current semantics are as follows:

  • FieldBundle adds a 4th dimension,
  • the 4th dimension can have any size,
  • the 4th dimension is always a float (from field_bundle.py, line 130)

While the 4th dimension is clearly stated everywhere, the Float type might surprise developers.

I fixed that. It was an actual bug because the data follows a Quantity which is not limited. Passed now as a optional field to the register. Better will read from the Quantity, but the systems of type & bundle are un-tethered for now so we have to rely on user not missing it, which was already the case before.

I do have one structural question: what happens if developers initialize a FieldBundle without a mapping? Won't that break the map() / index() (and by extension __getattr__()) functions?

Then the data is "blind", which means you can still:

  • Use it as a pure 4D array
  • Access 3D array by index with field.quantity[:,:,:, i]

This is not an issue as some code don't care about the association to bespoke sub-array (for example UW moves all tracers the same).

As for the __getattr__ I couldn't raise like I should have because of orchestration. The next best thing is to go None which will fail when accessed.

Apart from this, I think structurally, this can go in as-is for a first version. When we move to more than 4 dimensions, we can revisit the details. Might be easier once we have a bit of experience with the bundles. I left a few of nitpicks inside - nothing urgent. My wishlist (for future work) includes

  • FieldBundleType: docstrings for the two methods and unit test (register, re-register, and type resolving with/without markup)
  • FieldBundle docstrings

Docstrings done.
uTest... I am going under a tunnel :)

@romanc
Copy link
Copy Markdown
Collaborator

romanc commented May 16, 2025

I do have one structural question: what happens if developers initialize a FieldBundle without a mapping? Won't that break the map() / index() (and by extension __getattr__()) functions?

Then the data is "blind", which means you can still:

* Use it as a pure 4D array

* Access 3D array by index with `field.quantity[:,:,:, i]`

This is not an issue as some code don't care about the association to bespoke sub-array (for example UW moves all tracers the same).

Aha, I see. All good then. This makes sense to me and I think we should keep that flexibility.

@FlorianDeconinck FlorianDeconinck requested a review from romanc May 16, 2025 17:24
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.

LGTM!

@FlorianDeconinck FlorianDeconinck merged commit f287122 into NOAA-GFDL:develop May 16, 2025
5 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