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.

[hiker]

@sergisiso , @arporter - trying to progress this PR, here an alternative proposal, which feels more generic, and would give us many more options to pass/store information when lowering.

What about the idea of adding an optional dsl-info attribute to each symbol? This would be a class that could keep track of any DSL information (that we might otherwise lose when lowering), maybe just a glorified dictionary, or maybe a more intricate implementation.

Ideally, we could (for a given combination of alg-layer, psy-layer, kernel) share this instance information across symbols in the three symbol tables:

alg-layer:

   call invoke(my_kern(my_field))

In this situation we would mark that my_field has the dsl-name my_field. And e.g. we might store information if my_field is a local variable in the alg layer (not sure if this is useful, just as an example); or maybe that my_field is on a given space (because we can find code in the alg layer where we allocate my_field and can extract that information???).

psy-layer:

   subroutine invoke_my_kern(my_field)
   ...
       ! Initialise field and/or operator proxies
    my_field_proxy = my_field%get_proxy()
    my_field_data => my_field_proxy%data

We know that this my_field dummy argument is the my_field from the alg layer, so they can share attributes.

When assigning the proxy and data pointer, my_field_proxy and my_field_data could share the information object with the one for my_field dummy-argument (which is shared with the alg-layer). This means, even if we rename some of this due to name clashes, we still have the original DSL name, plus any additional information that was originally attached,

If required (and we module inline the kernel), this could then even be useful for kernel modifications (since we know at this stage that then kernel argument my_field which will be a Fortran array is originally based on the my_field.

I feel symbols might be the right level??? And might that work? Does this sound like a better and more generic solution to the problem of getting DSL names? I am happy to open a separate ticket for that, and do a test implementation to evaluate this?

[sergisiso]

@arporter @LonelyCat124 @hiker Is there currently a path from a psy-layer field to go it its argument in the alg layer representing it? Maybe we just need to follow that instead of storing state?

We also need it to validate if 2 ANY_SPACE fields that some from two different arguments are actually the same symbol.