3252 value check field names in gocean

[hiker]

This implements our discussed solution: a reference stores the 'dsl name' , which is then used during variable access handling / call tree analysis to provide this name to psy_data. This works for gocean so far.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a74dfdf) to head (da8011c).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3385   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          397       397           
  Lines        55577     55595   +18     
=========================================
+ Hits         55577     55595   +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hiker]

I have no idea why I never marked this one as ready, as far as I can tell it fixes the gocean issue of having to use fld_data instead of just fld in PSyData.

Note that I have split the value_range tests into two files (one for the transformation, one for the node).

IT triggered.

[sergisiso]

@hiker I have to admit that I don't see why we need to have the "data" compoment implied, we are chaning from:
PSY_VALUE_RANGE="main.init.b_fld%data=2:3"
to
PSY_VALUE_RANGE="main.init.b_fld=2:3"
but %data is not a DSL concept, is part of the infrastructure definition of what a field is.

What happens if we have a region that modifies the "tile" component of field. Could we still refer to it? And wouldn't
PSY_VALUE_RANGE="main.init.b_fld%data=2:3;main.init.b_fld%tile=1:4"
be more clear than having one of them implied?

Maybe the lfric proxies make a more convincing example of why we need this?

[hiker]

@hiker I have to admit that I don't see why we need to have the "data" compoment implied, we are chaning from: PSY_VALUE_RANGE="main.init.b_fld%data=2:3" to PSY_VALUE_RANGE="main.init.b_fld=2:3" but %data is not a DSL concept, is part of the infrastructure definition of what a field is.

I think we discussed this in #3252: imho, a user should not need to be aware of the details of the infrastructure library. Esp. for value_check, which is a more 'user level' functionality: a natural scientist wanting to ensure that a certain field has the expected values range. It's (imho) a useful debugging feature. I would envision that over time LFRic could collect a set of variable ranges, meaning that potential errors are flagged much earlier, before they can cause a crash elsewhere. Note that the idea of this feature was motivated by a ~2-3 weeks debugging session of the UM, which was ultimately caused by the 'ice fraction' of a cell not being in [0,1] (it was something in the millions, causing huge temperature changes, then high winds, which made the UM unstable).

From that point of view, I don't think a user should need to know about %data (nor any of the other members of that struct). I think it's one of the important points of PSyclone that natural scientists should only think in terms of (whole) fields (and not how they are implemented).

On the other hand, a more advanced developer might want to also check other fields for validity. Ideally, we would be able to support both notations (but that doesn't work atm, I would need to check how much effort it would be to support that.). The infrastructure supports that fine, i.e. if I pass in both:

    CALL value_range_check_psy_data % PreDeclareVariable("b_fld", b_fld % data)
    CALL value_range_check_psy_data % PreDeclareVariable("b_fld%data", b_fld % data)

then both names (b_fld and b_fld%data) can be used. So, all we need would be the transformation to provide the alternative name.

What happens if we have a region that modifies the "tile" component of field. Could we still refer to it? And wouldn't
PSY_VALUE_RANGE="main.init.b_fld%data=2:3;main.init.b_fld%tile=1:4" be more clear than having one of them implied?

From a (natural science) user's point of view, they should not need to know about tile (or data) imho.

Maybe the lfric proxies make a more convincing example of why we need this?

I think that's actually more an example of why we don't need this :)

PSyclone create a pointer called field1_data in the psy-layer for a user defined field1. So, the user has to use field1_data to specify the range. But (warning: stupid but in theory possible use case following :) ), if the user should (later) add a new kernel parameter called field1_data, the the pointer variable for field1 will be renamed to field1_data_1 (to avoid the name clash). That means, a user now has to use field1_data_1 to specify the range in the environment variable. Even worse, if this name is not changed in PSY_VALUE_RANGE, the settings for field1_data in the environment variable are now used to check an entirely different field. Now, this PR does not handle the LFRic issues, but would be the basis for fixing this