Test UnitsAspect update_payload procedures #4347

darianboggs · 2026-01-29T04:58:35Z

Types of change(s)

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Trivial change (affects only documentation or cleanup)
Refactor (no functional changes, no api changes)

Checklist

Tested this change with a run of GEOSgcm
Ran the Unit Tests (make tests)

Description

This PR:

Adds MAPL procedures to remove key/value pairs from Field and FieldBundle ESMF_Info objects. While MAPL procedures exist to get and set the values based on key, we need to be able to remove the pairs when the value is unset.
Updates UnitsAspect to use this in the update_payload procedures, for both Field and FieldBundle objects, when the UnitsAspect is_mirror (units not allocated).
Test UnitsAspect update_payload and update_payload_from procedures for Field and FieleBundle objects.

Related Issue

…date_from_payload - Tests for update_from_payload with ESMF_Field (R4, R8, I4, I8, MIRROR) - Tests for update_from_payload with ESMF_FieldBundle (R4, R8, MIRROR) - Tests for update_payload with ESMF_Field (R4, R8, I4, I8, MIRROR) - Tests for update_payload with ESMF_FieldBundle (R4, R8, MIRROR) - Round-trip tests to verify data integrity - Corner cases: no field/bundle provided, multiple consecutive updates - Tests for overwriting existing typekind values - Tests for proper mirror flag handling These tests ensure reliable operation of critical framework methods that synchronize TypekindAspect state with ESMF payloads.

- Add comprehensive test suite for TypekindAspect (19 tests) - Test update_payload() and update_from_payload() methods - Cover R4, R8, I4, I8, and MIRROR typekinds for Field and FieldBundle - Initialize with non-matching typekinds to avoid false positives - Refactor FieldBundleInfo to delegate typekind handling to FieldInfo - Removes duplicate to_string/to_TypeKind functions - Fixes MIRROR typekind support for bundles (was returning 200 instead of "<MIRROR>") - FieldInfoGetInternal/SetInternal now handle typekind parameter

- Created Test_UngriddedDimsAspect.pf with 18 tests covering: * Empty ungridded dimensions * Single and multiple ungridded dimensions * Mirror ungridded dimensions (documented FieldInfo layer bug) * Update operations in both directions (aspect ↔ payload) * Roundtrip and overwrite scenarios * Both Field and FieldBundle variants - Enhanced Test_UnitsAspect.pf from 4 to 14 tests covering: * Empty, simple, and compound units * Update operations in both directions * Roundtrip and overwrite scenarios * Both Field and FieldBundle variants - Test naming conventions follow TypekindAspect pattern - All tests passing (18/18 for UngriddedDims, 14/14 for Units) - Mirror tests include TODO notes documenting expected behavior once FieldInfo bug is resolved

tclune · 2026-01-29T12:56:32Z

field/FieldInfo.F90

+         namespace_ = namespace
+      end if
+
+      isPresent=present(units)


I think the name isPresent is serving too many roles in this procedure. But don't change it unless I find other things that I want changed.

Yes. I was concerned about that. I could have compressed it down to a single logical value like this:

isPresent = present(units) .and. units .and. ESMF_InfoIsPresent(info, key=full_key, _RC) if(isPresent) then ... end if

if I could force Fortran to evaluate it left to right. Even then, I needed a variable because for the _RC macro.

This may be out of date pending other decisions. If it stays in, I can reduce it down to a single assignment, which makes the block a little bit smaller. Unfortunately, I can't easily avoid the nested if blocks.

You cannot force Fortran to evaluate left-to right.

(Sorry - I missed your "if".)

tclune · 2026-01-29T12:57:48Z

field/FieldInfo.F90

+      integer :: i
+
+      child = ''
+      s = trim(full_key)


I think this line is unnecessary? Can just use full_key in the expressions below.

Yes. The trim isn't necessary, either. I can just use full_key instead of the extra s variable or trim(full_key). I can push this up with an update.

tclune · 2026-01-29T13:02:03Z

field/FieldRemove.F90

+
+contains
+
+   subroutine field_remove(field, unusable, units, standard_name, long_name, rc)


This feature is ultimately requiring a great deal of code for a relatively narrow corner case.

A bit late now, but I probably should have admitted that no one will ever set the mirror condition after having set it to something else. (Famous last words.)

More realistically it argues for always having a special value for the mirror case rather than using allocatable. My design overloads the responsibility of the ALLOCATABLE attribute: (1) used to indicate present/not-present and (2) mirror logic. (1) is a nice part of Fortran that works well here. (2) ... not so much.

The question then is do we want a derived type for Units (and a few other things in this category) or just a reserved string.

We will face the same issue soon with AttributesAspect, as StringVector() does not have a way to indicate mirror aside from not being allocated. (Empty string vector means "no relevant attributes".

Another issue I now see is what default do we want in VarSpec? When constructing a variable in a VarSpec, a missing "attributes" should mean empty attributes. A missing unit is more debatable, and probably currently shows as mirror. I think we need a consistent policy here, and that will drive the other stuff above. (I've known this concern was coming, but it was not a priority until now.)

It seems there are two values in question. The first is the units component for the UnitsAspect derived type. The second is the units key/value map in the payload ESMF_Info object. (Of course, this generalizes to other aspects and their components.) We could continue with our strategy for the derived type component, but change the strategy for the ESMF_Info for the payload.

UnitsAspect <=> Field Info

CURRENT units unallocated [is_mirror() ~ .TRUE.] <=> Missing units key

PROPOSED units unallocated [is_mirror() ~ .TRUE.] <=> units: NULL *

* In the Set direction, unallocated would map to units: NULL. In the Get direction, I think it would be better to map units: NULL to units unallocated and map missing units key to units unallocated as well. It would not be one-to-one, and purists would object, but I think it would be OK, since we don't need to recover the original Info object value once the units component is updated from the payload.

unallocated [is_mirror()] -> key: NULL
key: NULL -> units unallocated
missing units key -> units unallocated

I will think about this over lunch, and check on NULL in ESMF_Info objects. Also, I am thinking about a signal value to represent unset, but as we have seen before, we have to be very careful that the signal value will not collide with valid values. (So ``, unset and `NULL` (literal string) are not good.)

On further research, NULL may not be the right strategy. It would require calling ESMF_InfoSetNULL instead of ESMF_InfoSet (through MAPL_FieldSet). That adds complexity. I think it would be better to encode the unset status in the string that we read/write for units and similar Info objects. It would allow it to be handled differently for different components. We could have a character parameter variable that is shared. That would be our standard value in Info objects for character values (scalar) that are not set. In this case, it would look like this:

aspect%units not allocated -> units: UNSET

units: UNSET -> aspect%units not allocated
units not present -> aspect%units not allocated

The question remains what is the value of UNSET (or whatever variable name we come up with.) One possibility is ASCII NUL. It conveys a different meaning than "NULL" or "", and it is hard to imagine that a user would ever want to use that as a character value. We could use an unlikely combination of ASCII symbol characters, but we would need to choose carefully. In the context of units, it would not be difficult to pick that, but if we want a general Fortran character parameter variable to represent any unset character value in an Info object, we need to be more careful.

I have a followup question. But easier to ask by voice.

Well, maybe I can state it in writing. I think we still have to allow the units key to be simply missing in the info object. E.g., if someone uses a noncanonical construction of a Field, and never mention units, there will be no way to give it the "null" value in info in the first place.

Also for the MIRROR semaphore we have lots of options. lots of special characters cannot be used in actual units. (Should verify that.) So it might be useful to use a string that is humanly meaningful so that when you print the info you can understand it.

Currently we have "" meaning it has units but don't try converting them. And now we could have "". I'm quite open to the actual spelling here.

Note for me: We will use a string to indicate the mirror status. The string will be visible ASCII characters that are meaningful to humans, but it will include ASCII symbol characters to prevent collision with valid units. The characters will not be +, -, or _.

tclune

Technically I approve. But you'll see in the comments, that we may want to consider a different path here.

tclune and others added 7 commits January 28, 2026 21:29

Merged.

7313509

Remove file that was accidentally added

028f797

Tests pass except bundle update_payload empty

6c31aa1

All tests pass.

0f0e361

darianboggs requested a review from tclune January 29, 2026 04:58

darianboggs self-assigned this Jan 29, 2026

darianboggs added 🎁 New Feature This is a new feature 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 📈 MAPL3 MAPL 3 Related labels Jan 29, 2026

tclune reviewed Jan 29, 2026

View reviewed changes

tclune approved these changes Jan 29, 2026

View reviewed changes


		contains

		subroutine field_remove(field, unusable, units, standard_name, long_name, rc)

	UnitsAspect	<=>	Field Info
CURRENT	`units` unallocated [`is_mirror()` ~ .TRUE.]	<=>	Missing `units` key
PROPOSED	`units` unallocated [`is_mirror()` ~ .TRUE.]	<=>	`units: NULL` *

Test UnitsAspect update_payload procedures #4347

Are you sure you want to change the base?

Test UnitsAspect update_payload procedures #4347

Uh oh!

Conversation

darianboggs commented Jan 29, 2026

Types of change(s)

Checklist

Description

Related Issue

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tclune Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tclune left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tclune Jan 29, 2026 •

edited

Loading