Add optional attribute to Capgen #529

dustinswales · 2024-01-30T23:14:32Z

Description

This PR adds support to use the fortran optional attribute at the scheme level in Capgen. This functionality does not replace the active attribute, but rather builds on it.

Previously, conditionally allocated fields needed by a scheme were required to be accompanied with the allocation logic defined by the host (e.g the "active" condition). With this change, the scheme can internally query the "presence" instead of carrying the host model logic "down into the scheme".

The following modifications were made:

Metadata parsing: Check for compatibility between scheme and host metadata. If host has inactive variable, ensure that scheme has variable declared as optional.
Fortran parsing: Check for consistency between metadata and fortran file. If fortran file does not contain the optional attribute in its declaration, report error.
For any optional scheme variable, add optional attribute to cap level declarations.
Declare null pointer at cap level when host does not use optional scheme variable

Fixes:

Addresses #527 (new test for optional variable) and partially #526 (support for optional attribute in Capgen)

Testing:
test removed:
unit tests:
system tests: The var_compatibility_test was expanded, built on top of #525, to test this feature.
manual testing:

…onal_attr

dustinswales · 2024-01-31T18:49:49Z

@gold2718 @peverwhee @nusbaume
I think that this does almost all that we want it to do. Feel free to take a gander.
There's one piece that I'm not sure about? What needs to be done to add support to pass chunks of contiguous arrays? @climbfuji

climbfuji · 2024-01-31T18:55:46Z

@gold2718 @peverwhee @nusbaume I think that this does almost all that we want it to do. Feel free to take a gander. There's one piece that I'm not sure about? What needs to be done to add support to pass chunks of contiguous arrays? @climbfuji

In the caps, we need to declare a pointer for each variable with an active attribute that either points to a valid chunk of the contiguous array if the variable is active (based on the active condition), or is passed as a null pointer if the active-attribute condition is evaluated as .false.

…es/ccpp-framework into add_optional_attr

dustinswales · 2024-02-01T23:14:25Z

@gold2718 @DomHeinzeller I'm close to having this working, just one smallish hold up.

Do either of you two know how to change the local_name of a dummy argument in a call_list?
I need to go from, call sub(var=var_local) => call sub(var=var_local_ptr)

… into add_optional_attr

…t provided by host

dustinswales · 2024-02-08T18:41:02Z

@DomHeinzeller @gold2718 @peverwhee @nusbaume
This one is ready for review
I still need to chase down a failing unit test, but the system tests are working (I probably changed a function call and broke an interface in the unit tests)

dustinswales · 2024-02-08T18:45:26Z

Snippets of auto-generated caps:

climbfuji

This looks great! Only a few minor comments

scripts/suite_objects.py

test/var_compatibility_test/run_test

test/var_compatibility_test/test_host_data.meta

…readability

climbfuji

I have one doctest failure and a suggestion for a fix, other than that I am happy with the changes. (I ran doctests, all tests in test/ and also all tests in test_prebuild.

scripts/metavar.py

Co-authored-by: Dom Heinzeller <dom.heinzeller@icloud.com>

… into add_optional_attr

climbfuji

Thanks! doctests are now ok

peverwhee

Nice work, @dustinswales !

A couple comments/questions

scripts/ccpp_capgen.py

scripts/metavar.py

scripts/suite_objects.py

test/var_compatibility_test/effr_calc.meta

peverwhee

thanks @dustinswales !

climbfuji · 2024-03-05T03:08:21Z

@gold2718 Would you like to review the PR or should we go ahead and merge? Thanks!

climbfuji · 2024-03-06T16:00:32Z

We need to merge this so that I can bring feature/capgen down to main. That's needed, because ccpp_prebuild uses the capgen metadata parser and I need the updates for the mandatory optional attribute for schemes if the host model has an active attribute with a value other than the default .true.. Once I've got that update, I can make the optional var changes in prebuild similar to this PR for capgen, which will unblock the transition to contiguous arrays instead of blocked DDTs for the UFS.

gold2718 · 2024-03-06T16:16:36Z

We need to merge this so that I can bring feature/capgen down to main. That's needed, because ccpp_prebuild uses the capgen metadata parser and I need the updates for the mandatory optional attribute for schemes if the host model has an active attribute with a value other than the default .true.. Once I've got that update, I can make the optional var changes in prebuild similar to this PR for capgen, which will unblock the transition to contiguous arrays instead of blocked DDTs for the UFS.

Is there time to submit at least comments from partial reviews in progress?

climbfuji · 2024-03-06T16:17:27Z

We need to merge this so that I can bring feature/capgen down to main. That's needed, because ccpp_prebuild uses the capgen metadata parser and I need the updates for the mandatory optional attribute for schemes if the host model has an active attribute with a value other than the default .true.. Once I've got that update, I can make the optional var changes in prebuild similar to this PR for capgen, which will unblock the transition to contiguous arrays instead of blocked DDTs for the UFS.

Is there time to submit at least comments from partial reviews in progress?

Oh yes, for sure. Just wanted everyone to know why we need to get his in soon. Thanks for reviewing.

gold2718

This is not as thorough as usual for me, mainly because I have not found the time to go through the suite_objects logic in detail. However, it seems okay at the level I understand and the generated code is okay (meaning you get what you were looking for) so 👍
I do have a couple of questions and some suggestions if there is time for those.

gold2718 · 2024-02-26T14:07:18Z

test/var_compatibility_test/effr_calc.F90

+      real(kind_phys),    intent(in),optional    :: effrg_in(:,:)
+      real(kind_phys),    intent(in),optional    :: ncg_in(:,:)
+      real(kind_phys),    intent(out),optional   :: nci_out(:,:)
      real(kind_phys),    intent(inout) :: effrl_inout(:,:)
-      real(kind_phys),    intent(out)   :: effri_out(:,:)
+      real(kind_phys),    intent(out),optional   :: effri_out(:,:)
      real(8),intent(inout) :: effrs_inout(:,:)
-      logical,            intent(in)    :: has_graupel
+      real(kind_phys),    intent(out),optional   :: ncl_out(:,:)
      character(len=512), intent(out)   :: errmsg
      integer,            intent(out)   :: errflg


It is pretty non standard to have non-optional arguments follow optional arguments in a Fortran subprogram. This is because "there must be no further positional arguments after the first keyword argument."
This 'works' in the CCPP because we generate caps that use keywords for every argument (thanks @davegill), however, I think it is still good practice to write portable code.

I agree this is ugly, and will fix, but I believe the standard in this case is referring to the argument list, not the declarations within the procedure.

Right but for instance, errmsg and errflg are not optional but show up in the argument list after effrg_in, effri_out, and others.

This is something we've done consistently - always have errmsg and errflg at the very end. This is even more compelling if we think about the metadata order and the requirement (not sure if it's enforced in capgen, it isn't yet in prebuild) to have the same order as the argument list. Keeping those non-physics and purely framework related variables at the end would be nice.

Since nobody should be calling CCPP-compliant schemes directly - i.e. without ccpp-framework - we can rely on the arguments being listed as keywords?

Or, alternatively, move the framework-mandated variables to the very beginning? An awful lot of changes in the current schemes in the physics, but doable.

Since nobody should be calling CCPP-compliant schemes directly - i.e. without ccpp-framework - we can rely on the arguments being listed as keywords?

At the very least, make this a bold part of the documentation about optional variables. Anyone creating a new scheme can put the variables in any order and anyone is free to use current schemes outside of the CCPP but we should at least document the consequences.

I created issue #544 for that.

scripts/metavar.py

gold2718 · 2024-03-06T16:15:09Z

scripts/metavar.py

+                intent_str = 'allocatable         '
+                if target:
+                    intent_str = 'allocatable,'
+                    intent_str += '  target'
            else:
                intent_str = ' '*13
            # end if
        elif intent is not None:
            alloval = self.get_prop_value('allocatable')


Can this be True here given the elif allocatable above? If it can, then what if is also optional?

I'm not sure of the purpose of alloval here?
If we are in this block of code, the variable is not an allocatable?

scripts/suite_objects.py

Co-authored-by: goldy <1588651+gold2718@users.noreply.github.com>

… into add_optional_attr

climbfuji · 2024-03-08T15:34:09Z

I also created #545 so that I can go ahead and merge this.

dustinswales added 4 commits January 30, 2024 19:36

Add optional keyword. Work in progress.

964b776

Merge remote-tracking branch 'me/test_active_attribute' into add_opti…

bcdb37e

…onal_attr

Work in progress.

224fad3

Add optional attribute to varaible declarations in caps

aa652cd

dustinswales added 2 commits January 31, 2024 21:33

Merge branch 'test_active_attribute' of https://github.com/dustinswal…

462d08b

…es/ccpp-framework into add_optional_attr

Work in progress

9248678

dustinswales added 4 commits February 2, 2024 21:15

added active logic to pointer association

7a0c4c9

Merge branch 'feature/capgen' of https://github.com/NCAR/ccpp-framework…

a1a701b

… into add_optional_attr

Working now

423dc71

Added null pointers to caps when scheme has optional argument, but no…

aa59672

…t provided by host

dustinswales marked this pull request as ready for review February 8, 2024 18:39

dustinswales requested review from climbfuji, gold2718, mwaxmonsky, peverwhee, grantfirl and mkavulich as code owners February 8, 2024 18:39

climbfuji requested changes Feb 9, 2024

View reviewed changes

dustinswales added 7 commits February 12, 2024 21:30

Address reviewers comments. Add whitespace to autogenerated code for …

7b20265

…readability

Revert some changes

d7bad29

Add optional attribute to capgen test

9337a2e

Fix doctests.

3e4f88f

Fix doctest again

80da2a5

Fix doctest again again

c34496f

Fix doctest again again again

9125aae

dustinswales requested a review from climbfuji February 12, 2024 22:18

climbfuji requested changes Feb 21, 2024

View reviewed changes

scripts/metavar.py Show resolved Hide resolved

scripts/metavar.py Outdated Show resolved Hide resolved

dustinswales and others added 2 commits February 22, 2024 14:53

Update scripts/metavar.py

34aa7f5

Co-authored-by: Dom Heinzeller <dom.heinzeller@icloud.com>

Merge branch 'feature/capgen' of https://github.com/NCAR/ccpp-framework…

a9ae401

… into add_optional_attr

climbfuji approved these changes Feb 23, 2024

View reviewed changes

peverwhee reviewed Feb 26, 2024

View reviewed changes

grantfirl mentioned this pull request Feb 27, 2024

Dummy-allocate inactive local variables #536

Merged

climbfuji mentioned this pull request Mar 1, 2024

ccpp_prebuild: (re-)introduce optional attribute #540

Closed

Address reviewers comments

adad650

peverwhee approved these changes Mar 4, 2024

View reviewed changes

gold2718 approved these changes Mar 6, 2024

View reviewed changes

climbfuji mentioned this pull request Mar 6, 2024

bugfixes for multiple groups and scalar variable transforms #542

Merged

dustinswales and others added 4 commits March 6, 2024 10:03

Update scripts/suite_objects.py

00a905f

Co-authored-by: goldy <1588651+gold2718@users.noreply.github.com>

Update scripts/suite_objects.py

b4991be

Co-authored-by: goldy <1588651+gold2718@users.noreply.github.com>

Update scripts/suite_objects.py

b0ad2df

Co-authored-by: goldy <1588651+gold2718@users.noreply.github.com>

Merge branch 'feature/capgen' of https://github.com/NCAR/ccpp-framework…

809fc91

… into add_optional_attr

This was referenced Mar 8, 2024

Update CCPP tech doc: schemes can currently have optional arguments in between regular arguments - follow up with moving opt args to the end #544

Open

Follow up on formatting discussions in https://github.com/NCAR/ccpp-framework/pull/529 #545

Open

climbfuji merged commit c546cca into NCAR:feature/capgen Mar 8, 2024
10 checks passed

This was referenced Mar 8, 2024

Create tests for Optional attribute #527

Closed

Add optional attribute to Capgen/Prebuild #526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional attribute to Capgen #529

Add optional attribute to Capgen #529

dustinswales commented Jan 30, 2024 •

edited

Loading

dustinswales commented Jan 31, 2024

climbfuji commented Jan 31, 2024

dustinswales commented Feb 1, 2024

dustinswales commented Feb 8, 2024

dustinswales commented Feb 8, 2024

climbfuji left a comment

climbfuji left a comment

climbfuji left a comment

peverwhee left a comment

peverwhee left a comment

climbfuji commented Mar 5, 2024

climbfuji commented Mar 6, 2024

gold2718 commented Mar 6, 2024

climbfuji commented Mar 6, 2024

gold2718 left a comment

gold2718 Feb 26, 2024

dustinswales Mar 6, 2024

gold2718 Mar 6, 2024

climbfuji Mar 6, 2024

climbfuji Mar 6, 2024

gold2718 Mar 6, 2024

climbfuji Mar 8, 2024

gold2718 Mar 6, 2024

dustinswales Mar 6, 2024

climbfuji commented Mar 8, 2024

Add optional attribute to Capgen #529

Add optional attribute to Capgen #529

Conversation

dustinswales commented Jan 30, 2024 • edited Loading

Description

Fixes:

dustinswales commented Jan 31, 2024

climbfuji commented Jan 31, 2024

dustinswales commented Feb 1, 2024

dustinswales commented Feb 8, 2024

dustinswales commented Feb 8, 2024

climbfuji left a comment

Choose a reason for hiding this comment

climbfuji left a comment

Choose a reason for hiding this comment

climbfuji left a comment

Choose a reason for hiding this comment

peverwhee left a comment

Choose a reason for hiding this comment

peverwhee left a comment

Choose a reason for hiding this comment

climbfuji commented Mar 5, 2024

climbfuji commented Mar 6, 2024

gold2718 commented Mar 6, 2024

climbfuji commented Mar 6, 2024

gold2718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

climbfuji commented Mar 8, 2024

dustinswales commented Jan 30, 2024 •

edited

Loading