Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168 and #169) #150

climbfuji · 2023-12-27T03:53:04Z

Description

This PR is required to address issues with a recent PR to the physics (#75) that was not fully CCPP compliant. It introduced Fortran attributes optional and pointer in the GFS_suite_stateout_update scheme, both of which are not permitted/supported by the current CCPP framework.

The correct way to handle this situation, as described in the CCPP technical documentation and our recent paper (https://gmd.copernicus.org/articles/16/2235/2023/) is to pass the logical flag that is used in the active attribute and read/write from/to the arrays based on that flag.

I will note that we need to address the way we handle inactive data in the near future and that this may or may not require (re-)introducing Fortran attributes thrown out here, but as of now the current code is invalid per CCPP requirements.

I also pulled in #168. and #169 per request from the repository managers. Please see these issues for the description of the respective changes.

Issues

This PR fixes #75 (comment)

Dependencies / associated PRs

This PR is part of a set of PRs that need to be tested and merged together:

Note that the CI tests fail due to an error unrelated to this PR (related to the CI scripts themselves): https://github.com/ufs-community/ccpp-physics/actions/runs/7334889268/job/19972125491?pr=150

…' attributes

… into bugfix/ozphys_ccpp_compliant

dustinswales

@climbfuji Thanks for making this change.
My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics).
The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

climbfuji · 2024-01-02T16:19:21Z

@climbfuji Thanks for making this change. My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics). The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

Are these changes ready (everything allocated to at least size 0)?

dustinswales · 2024-01-02T16:22:17Z

@climbfuji Thanks for making this change. My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics). The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

Are these changes ready (everything allocated to at least size 0)?

Not yet, but very soon I believe. @grantfirl Can you confirm?

grantfirl · 2024-01-02T16:44:01Z

@climbfuji Thanks for making this change. My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics). The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

Are these changes ready (everything allocated to at least size 0)?

Not yet, but very soon I believe. @grantfirl Can you confirm?

Yes, I have code with all conditionally-allocated variables now being allocated to size 0, but I ran into issues before the break with respect to switching variables from pointers to allocatables. GFS_diagnostics is assigning pointers to components of DDTs. This works fine with the DDT components being pointers, and it is apparently supposed to work fine with allocatables as long as the top level DDT (GFS_data_type in this case) is given the TARGET attribute (which it already has), but the compiler is still erring out and I don't really understand why. If we need this ASAP, I can put everything back to pointers that are still being allocated to zero size, but I was going to try to debug why it won't work as TARGET allocatables in GFS_diagnostics.

climbfuji · 2024-01-02T16:46:25Z

@climbfuji Thanks for making this change. My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics). The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

Are these changes ready (everything allocated to at least size 0)?

Not yet, but very soon I believe. @grantfirl Can you confirm?

Yes, I have code with all conditionally-allocated variables now being allocated to size 0, but I ran into issues before the break with respect to switching variables from pointers to allocatables. GFS_diagnostics is assigning pointers to components of DDTs. This works fine with the DDT components being pointers, and it is apparently supposed to work fine with allocatables as long as the top level DDT (GFS_data_type in this case) is given the TARGET attribute (which it already has), but the compiler is still erring out and I don't really understand why. If we need this ASAP, I can put everything back to pointers that are still being allocated to zero size, but I was going to try to debug why it won't work as TARGET allocatables in GFS_diagnostics.

We can discuss this at our next CCPP framework meeting, but: Even if we add the size-0 allocation, this is only a duct-tape solution so that the code compiles and runs without errors and warnings, as required by NCEP operations.

This does not mean that we should throw overboard the rules and requirements that the CCPP framework developers came up with in the past, which are that conditionally allocated variables must have an active attribute (condition for allocation) in the host model metadata, and schemes must test for that condition in order to access the variable. If we want to change that, then I think we need to take this back to the CCPP framework developer meeting and discuss this in a broader context than just the UFS and NCEP operations. I personally think that we need to take this back to the drawing board and see if we can come up with a better solution altogether. But, for now, this PR is simply applying current CCPP rules and requirements and removing code that violates those rules.

dustinswales · 2024-01-02T19:58:24Z

@climbfuji Thanks for making this change. My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics). The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

Are these changes ready (everything allocated to at least size 0)?

Not yet, but very soon I believe. @grantfirl Can you confirm?

Yes, I have code with all conditionally-allocated variables now being allocated to size 0, but I ran into issues before the break with respect to switching variables from pointers to allocatables. GFS_diagnostics is assigning pointers to components of DDTs. This works fine with the DDT components being pointers, and it is apparently supposed to work fine with allocatables as long as the top level DDT (GFS_data_type in this case) is given the TARGET attribute (which it already has), but the compiler is still erring out and I don't really understand why. If we need this ASAP, I can put everything back to pointers that are still being allocated to zero size, but I was going to try to debug why it won't work as TARGET allocatables in GFS_diagnostics.

We can discuss this at our next CCPP framework meeting, but: Even if we add the size-0 allocation, this is only a duct-tape solution so that the code compiles and runs without errors and warnings, as required by NCEP operations.

There are other solutions that could be applied instead of allocating to size(0):

Declaring arguments as explicit-shape vs. assumed shape OR
adding the optional attribute.
would both remove the runtime errors, but they aren't CCPP compliant. Allocating to size(0) is allowable within the CCPP rules, but as you said, and I completely agree, it's just duct-tape to avoid runtime errors.

This does not mean that we should throw overboard the rules and requirements that the CCPP framework developers came up with in the past, which are that conditionally allocated variables must have an active attribute (condition for allocation) in the host model metadata, and schemes must test for that condition in order to access the variable. If we want to change that, then I think we need to take this back to the CCPP framework developer meeting and discuss this in a broader context than just the UFS and NCEP operations. I personally think that we need to take this back to the drawing board and see if we can come up with a better solution altogether. But, for now, this PR is simply applying current CCPP rules and requirements and removing code that violates those rules.

I agree that this needs further discussion. TBH, I never really understood the spirit of the "active" attribute and I think it should be revisited.

climbfuji · 2024-01-02T20:00:09Z

@climbfuji Thanks for making this change. My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics). The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

Are these changes ready (everything allocated to at least size 0)?

Not yet, but very soon I believe. @grantfirl Can you confirm?

Yes, I have code with all conditionally-allocated variables now being allocated to size 0, but I ran into issues before the break with respect to switching variables from pointers to allocatables. GFS_diagnostics is assigning pointers to components of DDTs. This works fine with the DDT components being pointers, and it is apparently supposed to work fine with allocatables as long as the top level DDT (GFS_data_type in this case) is given the TARGET attribute (which it already has), but the compiler is still erring out and I don't really understand why. If we need this ASAP, I can put everything back to pointers that are still being allocated to zero size, but I was going to try to debug why it won't work as TARGET allocatables in GFS_diagnostics.

We can discuss this at our next CCPP framework meeting, but: Even if we add the size-0 allocation, this is only a duct-tape solution so that the code compiles and runs without errors and warnings, as required by NCEP operations.

This does not mean that we should throw overboard the rules and requirements that the CCPP framework developers came up with in the past, which are that conditionally allocated variables must have an active attribute (condition for allocation) in the host model metadata, and schemes must test for that condition in order to access the variable. If we want to change that, then I think we need to take this back to the CCPP framework developer meeting and discuss this in a broader context than just the UFS and NCEP operations. I personally think that we need to take this back to the drawing board and see if we can come up with a better solution altogether. But, for now, this PR is simply applying current CCPP rules and requirements and removing code that violates those rules.

@climbfuji Thanks for making this change. My intention was to a) avoid using host logic to control scheme diagnostics, and b) add more flexibility over the diagnostics being requested (e.g output only a subset of the available diagnostics). The idea being that if the host allocates (associates) them, the scheme knows to output them. But now that everything will be allocated to at least size(0), this logic doesn't work. We could change "if associated(var)" with "if size(var) > 0" or "if size(var) > 0" ? Thoughts?

Are these changes ready (everything allocated to at least size 0)?

Not yet, but very soon I believe. @grantfirl Can you confirm?

Yes, I have code with all conditionally-allocated variables now being allocated to size 0, but I ran into issues before the break with respect to switching variables from pointers to allocatables. GFS_diagnostics is assigning pointers to components of DDTs. This works fine with the DDT components being pointers, and it is apparently supposed to work fine with allocatables as long as the top level DDT (GFS_data_type in this case) is given the TARGET attribute (which it already has), but the compiler is still erring out and I don't really understand why. If we need this ASAP, I can put everything back to pointers that are still being allocated to zero size, but I was going to try to debug why it won't work as TARGET allocatables in GFS_diagnostics.

We can discuss this at our next CCPP framework meeting, but: Even if we add the size-0 allocation, this is only a duct-tape solution so that the code compiles and runs without errors and warnings, as required by NCEP operations.

There are other solutions that could be applied instead of allocating to size(0):

Declaring arguments as explicit-shape vs. assumed shape OR

adding the optional attribute.
would both remove the runtime errors, but they aren't CCPP compliant. Allocating to size(0) is allowable within the CCPP rules, but as you said, and I completely agree, it's just duct-tape to avoid runtime errors.

This does not mean that we should throw overboard the rules and requirements that the CCPP framework developers came up with in the past, which are that conditionally allocated variables must have an active attribute (condition for allocation) in the host model metadata, and schemes must test for that condition in order to access the variable. If we want to change that, then I think we need to take this back to the CCPP framework developer meeting and discuss this in a broader context than just the UFS and NCEP operations. I personally think that we need to take this back to the drawing board and see if we can come up with a better solution altogether. But, for now, this PR is simply applying current CCPP rules and requirements and removing code that violates those rules.

I agree that this needs further discussion. TBH, I never really understood the spirit of the "active" attribute and I think it should be revisited.

I agree!

…bles not given an explicit value" warning. The following table provides justification for setting the variable to 0, it is how they are treated in other places. | file | line | description | |--------------------+------+-----------------------------| | ugwpv1_gsldrag.F90 | 521 | zlwb(:)= 0. ; zogw(:)=0. | | ugwp_driver_v0.F | 206 | zmtb(i) = 0.0 | | cires_ugwp.F90 | 297 | if (do_ugwp) zlwb(:) = 0. |

… drain_cpl and dsnow_cpl vars are changed from intent(out) to intent(in) variables. This is to replicate the rain_cpl and snow_cpl variables.

…out) variables. Variable err_message will report any errors in open and read statements

… into bugfix/ozphys_ccpp_compliant

…es/ccpp-physics into bugfix/ozphys_ccpp_compliant

…eir related meta files

grantfirl · 2024-02-21T02:02:11Z

@climbfuji Could you pull in #169? I've run RTs separately on it, and those changes also don't affect baselines. That way, we'll merge 3 PRs in one go.

…hysics into bugfix/ozphys_ccpp_compliant

climbfuji · 2024-02-21T03:42:05Z

@grantfirl done. I also updated my set of PRs from the authoritative repositories and pushed the changes to all PRs.

zach1221 · 2024-02-23T16:47:53Z

@grantfirl testing is complete on ufs-wm PR #2066. Can you please merge this PR?
fyi @climbfuji

grantfirl · 2024-02-23T16:53:58Z

@dustinswales @climbfuji Thanks for merging, Dustin -- beat me to it. I'm glad to get these changes in finally.

…run compile jobs of regression test suite only & -mcmodel=medium gnu.cmake option (also includes #2142) (#2066) * UFSWM - Add option to `rt.sh` to run compile-only tests * FV3 - Update submodule pointer (NOAA-EMC/fv3atm#747) * ccpp-physics - Update submodule pointer (ufs-community/ccpp-physics#150) * ccpp-framework - Update submodule pointer (NCAR/ccpp-framework#519) * Update WM license and documentation logo * Update GNU.cmake: -mcmodel=medium

Make ozone physics CCPP compliant by removing 'optional' and 'pointer…

790960e

…' attributes

climbfuji mentioned this pull request Dec 28, 2023

ccpp_prebuild.py: add support for chunked arrays NCAR/ccpp-framework#519

Merged

Merge branch 'ufs/dev' of https://github.com/ufs-community/ccpp-physics…

2470634

… into bugfix/ozphys_ccpp_compliant

climbfuji marked this pull request as ready for review December 28, 2023 22:32

climbfuji requested review from grantfirl, Qingfu-Liu, dustinswales and mzhangw as code owners December 28, 2023 22:32

climbfuji self-assigned this Dec 28, 2023

dustinswales requested changes Jan 2, 2024

View reviewed changes

dustinswales approved these changes Jan 2, 2024

View reviewed changes

scrasmussen and others added 5 commits January 10, 2024 14:33

issue 1984 fix: "intent out variables not given a value" warning. The…

f183508

… drain_cpl and dsnow_cpl vars are changed from intent(out) to intent(in) variables. This is to replicate the rain_cpl and snow_cpl variables.

issue 1984 fix: initialize err_message to "" because they are intent(…

ba7c627

…out) variables. Variable err_message will report any errors in open and read statements

Merge branch 'ufs/dev' of https://github.com/ufs-community/ccpp-physics…

31b4dc8

… into bugfix/ozphys_ccpp_compliant

Removing a few variables that are not used in the *_pre function

65bf1d0

Qingfu-Liu approved these changes Jan 12, 2024

View reviewed changes

Merge branch 'ufs/dev' of https://github.com/ufs-community/ccpp-physics…

e2ca394

… into bugfix/ozphys_ccpp_compliant

grantfirl mentioned this pull request Jan 22, 2024

Fix -check all using zero-size allocations #159

Closed

grantfirl approved these changes Jan 24, 2024

View reviewed changes

Fix .gitmodule

fd71b22

grantfirl mentioned this pull request Feb 7, 2024

Fix .gitmodule bug #168

Merged

climbfuji added 2 commits February 7, 2024 10:23

Merge branch 'ufs/dev' of https://github.com/ufs-community/ccpp-physics…

2c6003e

… into bugfix/ozphys_ccpp_compliant

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

877bb1f

…es/ccpp-physics into bugfix/ozphys_ccpp_compliant

climbfuji changed the title ~~Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes~~ Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168) Feb 7, 2024

climbfuji linked an issue Feb 7, 2024 that may be closed by this pull request

Erroneous .gitmodules #165

Closed

mzhangw approved these changes Feb 7, 2024

View reviewed changes

grantfirl mentioned this pull request Feb 9, 2024

Issue 1984 fix: reduce warnings for ccpp-physics #149

Closed

Qingfu-Liu added 2 commits February 9, 2024 13:45

remove unused variables from files unified_ugwp.F90, dcyc2t3.f and th…

afa4f6e

…eir related meta files

remove unused variables from files dcyc2t3.f and dcyc2t3.meta

82c6873

grantfirl mentioned this pull request Feb 20, 2024

Remove warnings from CCPP physics #169

Merged

Merge branch 'qingfu_changes' of https://github.com/qingfu-liu/ccpp-p…

c468c66

…hysics into bugfix/ozphys_ccpp_compliant

climbfuji changed the title ~~Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168)~~ Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168 and #169) Feb 21, 2024

dustinswales merged commit 4ac3009 into ufs-community:ufs/dev Feb 23, 2024
3 checks passed

grantfirl mentioned this pull request Feb 27, 2024

[production/RRFS.v1] Fix 'check all' with 0-size allocations #178

Merged

grantfirl mentioned this pull request Mar 28, 2024

UFS-dev PR#173 NCAR/ccpp-physics#1065

Merged

grantfirl mentioned this pull request May 14, 2024

On branch jm-pr-multiple-instances-of-ccpp-physics NCAR/ccpp-physics#1000

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168 and #169) #150

Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168 and #169) #150

climbfuji commented Dec 27, 2023 •

edited

Loading

dustinswales left a comment

climbfuji commented Jan 2, 2024

dustinswales commented Jan 2, 2024

grantfirl commented Jan 2, 2024

climbfuji commented Jan 2, 2024

dustinswales commented Jan 2, 2024

climbfuji commented Jan 2, 2024

grantfirl commented Feb 21, 2024

climbfuji commented Feb 21, 2024

zach1221 commented Feb 23, 2024

grantfirl commented Feb 23, 2024

Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168 and #169) #150

Make ozone physics CCPP compliant by removing 'optional' and 'pointer' attributes (includes #168 and #169) #150

Conversation

climbfuji commented Dec 27, 2023 • edited Loading

Description

Issues

Dependencies / associated PRs

dustinswales left a comment

Choose a reason for hiding this comment

climbfuji commented Jan 2, 2024

dustinswales commented Jan 2, 2024

grantfirl commented Jan 2, 2024

climbfuji commented Jan 2, 2024

dustinswales commented Jan 2, 2024

climbfuji commented Jan 2, 2024

grantfirl commented Feb 21, 2024

climbfuji commented Feb 21, 2024

zach1221 commented Feb 23, 2024

grantfirl commented Feb 23, 2024

climbfuji commented Dec 27, 2023 •

edited

Loading