Declare constants at the native precision of MPAS #1282
Conversation
When building MPAS as a dycore, certain constants in the `mpas_constants` module are
imported from the `physconst` module, which is a part of CAM/CAM-SIMA. However,
multiple issues arise if the precision of those constants differs from MPAS.
For example, building MPAS in single precision mode with CAM-SIMA fails due to
multiple occurrences of type mismatch between actual and dummy arguments.
mpas_geometry_utils.F:885:157:
885 | call mpas_log_write('$r', MPAS_LOG_ERR, realArgs=(/mpas_triangle_signed_area_sphere(a,b,c,sphereRadius) - pii/2.0_RKIND*sphereRadius*sphereRadius/))
| 1
Error: Type mismatch in argument 'realargs' at (1); passed REAL(8) to REAL(4)
Here, `pii` is declared by CAM-SIMA to be double precision, and it causes unintended
floating-point promotion in the expression.
The solution is to ensure that constants in the `mpas_constants` module are declared
at the native precision of MPAS.
Enforce `implicit none` at the module level so it applies everywhere within. It is also a best practice in Fortran.
…e level A blanket `use` statement imports all public entities from that module. For the case here, it causes namespace pollution at the module level. Also, it is difficult to tell where the imported entities come from. Therefore, `use` statements should always include the `only` option. It is also a best practice in Fortran.
It is a best practice in Fortran to always specify the accessibility of module variables (as well as procedures). For the case here, `RKIND` is imported from `mpas_kind_types`. Due to the lack of accessibility attributes, other modules that `use mpas_constants` also have access to `RKIND`. Therefore, avoid causing unexpected namespace pollution to other modules by specifying accessibility attributes.
| implicit none | ||
|
|
||
| public | ||
| private :: RKIND |
There was a problem hiding this comment.
Is there a significance for this line?
There was a problem hiding this comment.
I think this question might also be answered by the commit message, quoted below:
It is a best practice in Fortran to always specify the accessibility of module variables (as well as procedures).
For the case here,
RKINDis imported frommpas_kind_types. Due to the lack of accessibility attributes, other modules thatuse mpas_constantsalso have access toRKIND.Therefore, avoid causing unexpected namespace pollution to other modules by specifying accessibility attributes.
| @@ -51,15 +57,14 @@ module mpas_constants | |||
|
|
|||
| contains | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
We may want this newline preserved I think
| @@ -74,9 +79,25 @@ module mpas_constants | |||
| !----------------------------------------------------------------------- | |||
| subroutine mpas_constants_compute_derived() | |||
|
|
|||
| implicit none | |||
|
|
|||
There was a problem hiding this comment.
Is there a specific reason for removing the implicit none here?
There was a problem hiding this comment.
Looking at the commit history, my question is answered
There was a problem hiding this comment.
Yes, implicit none has been moved to the module level so it applies everywhere within.
When building MPAS as a dynamical core, certain constants in the
mpas_constantsmodule are imported from thephysconstmodule, which is a part of CAM/CAM-SIMA. However, multiple issues arise if the precision of those constants differs from MPAS.For example, building MPAS in single precision mode with CAM-SIMA fails due to multiple occurrences of type mismatch between actual and dummy arguments.
Here,
piiis declared by CAM-SIMA to be double precision, and it causes unintended floating-point promotion in the expression.The solution is to ensure that constants in the
mpas_constantsmodule are declared at the native precision of MPAS. Note that CAM does not support running MPAS in single precision mode at all, so the above build error cannot be reproduced.In addition, some best practices of modern Fortran have been implemented.
For stand-alone MPAS, the compiled executables stay bitwise identical, with or without this PR. No further tests are needed. For CAM/CAM-SIMA, it generates bitwise identical model results, with or without this PR. Additionally for CAM-SIMA, its regression tests all pass, indicating identical model results to the previous baseline.