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 (d9515d0) to head (416c131).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3385   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          399       399           
  Lines        56094     56112   +18     
=========================================
+ Hits         56094     56112   +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

[arporter]

I feel a bit bad that I haven't got to this @hiker but I thought I wouldn't start a review just as I'm about to go on leave. I've added @LonelyCat124 as a potential reviewer as it might aid him in learning more about the DSL side of things :-)

[sergisiso]

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 :)

Sorry for the confusion, but by "why we need this" I meant the use of psy_layer field names as you are proposing. And indeed the lfric example with renaming that you explain in your comment is more compelling.

Another question then is if it would make more sense to apply the transformation to the algorithm layer then? Since this is what the scientist has as their point of reference.

[hiker]

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 :)

Sorry for the confusion, but by "why we need this" I meant the use of psy_layer field names as you are proposing. And indeed the lfric example with renaming that you explain in your comment is more compelling.

Another question then is if it would make more sense to apply the transformation to the algorithm layer then? Since this is what the scientist has as their point of reference.

Esp. value range check is something you would want to apply for each kernel (to identify which kernel causes invalid values). As far as I can see, on the algorithm layer we can only instrument invokes, i.e. potentially large sequences of kernels calls.

[arporter]

The basic implementation is quite simple but I am wondering whether putting 'DSL' information into a generic PSyIR class (or two) is the right way to go? This is the same kind of issue that Rupert was trying to solve in capturing the semantics of arguments passed into Kernels. I was wondering whether the PSy-layer routine should have some sort of map to capture this information as that's where it belongs. However, that might then finding the necessary information difficult in the various places where it's used.