Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fms mixedmode #857

Merged
merged 22 commits into from
Jan 20, 2022
Merged

Fms mixedmode #857

merged 22 commits into from
Jan 20, 2022

Conversation

MinsukJi-NOAA
Copy link
Contributor

@MinsukJi-NOAA MinsukJi-NOAA commented Nov 5, 2021

Description
This PR includes code changes to allow a single FMS library to handle running 32-bit FV3 and 64-bit MOM6 for the coupled UFS weather model. The approach taken to minimize the code changes is to assume FMS is compiled in 64 bit, and to only modify the interfaces to FV3.

How Has This Been Tested?

  • Code changes pass the FMS CI tests (GNU) on GitHub Actions.
  • UFS weather model regression tests have been run on Hera using Intel compiler with expected results
    • 64 bit tests all reproduce the baseline
    • 32 bit tests complete, but do not reproduce the baseline

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes: make -k check resulted in no FAIL or ERROR on Hera platform with intel and impi 2020.2

MinsukJi-NOAA and others added 16 commits October 5, 2021 15:45
Code changes for files in the `diag_manager` directory
* Modify set_tracer_profile in tracer_manager.F90
* Modify real_to_time_type in time_manager.F90
* Modify set_tracer_profile in tracer_manager.F90
* Specify platform_mod variables used
* Modify real_to_time_type in time_manager.F90
* Modify files in the sat_vapor_pres directory
* Fix select type statement in sat_vapor_pres_k.F90
* Move write temp statement inside select type
* add constants4 directory for single precision constants
* modify CMakeLists.txt
* modify constants.F90 and fmsconstants.F90
* modify constants4/constants4.F90 and constants4/fmsconstants4.F90
* update constants4/fmsconstants4.F90
…3 for atmos_4xdaily* file NaN values (#7)

* Modify sat_vapor_pres_k.F90 to fix FV3 32 bit atmos_4xdaily* file NaN values
…t of triple do loops (#8)

* Move SELECT TYPE blocks out of triple do loops in diag_manager.F90
@MinsukJi-NOAA
Copy link
Contributor Author

I am looking into why additional file changes are showing up in the PR.

@MinsukJi-NOAA
Copy link
Contributor Author

I am looking into why additional file changes are showing up in the PR.

This issue is resolved and the PR is ready for review.

@MinsukJi-NOAA
Copy link
Contributor Author

Based on EMC's testing with the coupled UFS weather model, MOM6 requires the following one file changes in order to successfully call send_data routines in diag_manager: NOAA-EMC/MOM6#79

@MinsukJi-NOAA
Copy link
Contributor Author

Dycore running in 32 bit requires the code changes made here: https://github.com/NOAA-EMC/GFDL_atmos_cubed_sphere/tree/fms_mixedmode

Copy link
Contributor

@rem1776 rem1776 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments in the code that apply to routines throughout but the biggest thing is to have class default with error calls to make sure any invalid non-real types can't be passed in to any public routines, as well as checking the kind sizes match up when needed.

The new constants4 directory should probably be added to the autotools build system as well. You can probably just copy over the Makefile.am from the original constants folder to constants4 with a few minimal changes to the file/library names, and then just add them to the root Makefile.am and configure.ac.

@MinsukJi-NOAA
Copy link
Contributor Author

MinsukJi-NOAA commented Nov 12, 2021

Code changes (9242fe4) were made to address the reviewer's comments. Checking if all class(*) arguments are of the same real kind is done only if these arguments are used together (see sat_vapor_pres_k.F90). Otherwise, if each argument is used by itself, only case default is done for each argument.

Copy link
Member

@thomas-robinson thomas-robinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 5 themes for my change requests (I used the *4 and *8 format for shorthand):

  1. I am slightly worried about implicit casting generating warnings. Per the style guide, new code must not generate any new warnings. There is a lot of
real :: r
class(*) :: x
select type (x)
  type is (real*4)
    r = x
  type is (real*8)
    r = x
end select

What happens if these types don't match? Will there be a warning? Are these guaranteed to match, and will the compiler know that?
2. This should be 1.1 because it's basically the same thing. There is code that looks like

class(*) :: x
select type (x)
  type is (real*4)
    x = 100.
  type is (real*8)
    x = 100.
end select

I think you want to make sure your values are represented correctly. You either need to include the correct amount of digits after the decimal, use an e or d, or cast the number to the correct precision

class(*) :: x
select type (x)
  type is (real*4)
    x = 100.e0
  type is (real*8)
    x = 100.d0
end select

I think this should work
3. As someone who has had to deal with a vague "unsupported type" as an error, I am very impartial to using something that lacks detail as an error message. The error message should at a minimum say that the type of the variable is not real(kind=4) or real(kind=8) and it must be changed. You don't have to have the variable name like I added for each error message if you want to do a large replace, but please make the error messages more descriptive.
4. Where you check multiple variables to see if their types match, you need a different error message. Those variables could be supported kinds, but they are not matching which causes an error. Again, you don't need the variable names like I included, but you should have something that better explains the error.
5. All of the new variables need to be documented with doxygen. Most of the new variables is valid_types. I think I have a sed command you could use to add the comments for valid_types, but you should make sure it works.

sed -i 's/logical :: valid_types = .false./logical :: valid_types = .false. !< For checking if variable types match/g' *

Copy link
Member

@thomas-robinson thomas-robinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 5 themes for my change requests (I used the *4 and *8 format for shorthand):

  1. I am slightly worried about implicit casting generating warnings. Per the style guide, new code must not generate any new warnings. There is a lot of
real :: r
class(*) :: x
select type (x)
  type is (real*4)
    r = x
  type is (real*8)
    r = x
end select

What happens if these types don't match? Will there be a warning? Are these guaranteed to match, and will the compiler know that?
2. This should be 1.1 because it's basically the same thing. There is code that looks like

class(*) :: x
select type (x)
  type is (real*4)
    x = 100.
  type is (real*8)
    x = 100.
end select

I think you want to make sure your values are represented correctly. You either need to include the correct amount of digits after the decimal, use an e or d, or cast the number to the correct precision

class(*) :: x
select type (x)
  type is (real*4)
    x = 100.e0
  type is (real*8)
    x = 100.d0
end select

I think this should work
3. As someone who has had to deal with a vague "unsupported type" as an error, I am very impartial to using something that lacks detail as an error message. The error message should at a minimum say that the type of the variable is not real(kind=4) or real(kind=8) and it must be changed. You don't have to have the variable name like I added for each error message if you want to do a large replace, but please make the error messages more descriptive.
4. Where you check multiple variables to see if their types match, you need a different error message. Those variables could be supported kinds, but they are not matching which causes an error. Again, you don't need the variable names like I included, but you should have something that better explains the error.
5. All of the new variables need to be documented with doxygen. Most of the new variables is valid_types. I think I have a sed command you could use to add the comments for valid_types, but you should make sure it works.

sed -i 's/logical :: valid_types = .false./logical :: valid_types = .false. !< For checking if variable types match/g' *

* Make 'unsupported kind' error messages more descriptive. Change author of constants4.F90

* Add doxygen comment to valid_types in sat_vapor_pres/sat_vapor_pres_k.F90
* Modify codes for r8-r4 conversion to remove compiler warnings
* Remove OVERLOAD_R8 directives regarding send_data_*d_r8 subroutines
* Add Doxygen comments to constants4.F90
@MinsukJi-NOAA
Copy link
Contributor Author

@thomas-robinson I believe all raised issues have been addressed.

Regarding the r4-r8 conversion issue, the following approach has been taken.

When a real local variable is copied onto a class(*) dummy argument with intent(out):

subroutine test(x)
  class(*), intent(out) :: x
  real :: a
  ...
  select type (x)
  type is (real(4))
    x = real(a, kind=4)
  type is (real(8))
    x = a
  end select
  ...
end subroutine test

When a class(*) dummy argument with intent(in) is copied onto a real local variable:

subroutine test(x)
  class(*), intent(in) :: x
  real :: a
  ...
  select type (x)
  type is (real(4))
    a = x
  type is (real(8))
    a = real(x)
  end select
  ...
end subroutine test

@thomas-robinson
Copy link
Member

@MinsukJi-NOAA please check the boxes at the top of the PR indicating at you have done a self check and performed all of the tasks. If a box is not applicable, please check it anyway. Once that's done, we can merge and begin testing.

@MinsukJi-NOAA
Copy link
Contributor Author

@MinsukJi-NOAA please check the boxes at the top of the PR indicating at you have done a self check and performed all of the tasks. If a box is not applicable, please check it anyway. Once that's done, we can merge and begin testing.

@thomas-robinson Do I need to bring this branch up to date with GFDL's FMS main branch? Right now, this branch includes the changes up to Dec 13, 2021 (6b12cb5).

@rem1776
Copy link
Contributor

rem1776 commented Jan 20, 2022

@thomas-robinson Do I need to bring this branch up to date with GFDL's FMS main branch? Right now, this branch includes the changes up to Dec 13, 2021 (6b12cb5).

None of the current main's changes conflict with this PR, so you don't have to. I'll merge this in now.

@rem1776 rem1776 merged commit 428542f into NOAA-GFDL:emc_mixedmode Jan 20, 2022
rem1776 added a commit that referenced this pull request Jan 20, 2022
rem1776 pushed a commit that referenced this pull request Jan 20, 2022
* Change send_data interface and any necessary routines for mixed real precision support
* Adds an r4 size constants file

BREAKING CHANGE: changes some argument types in diag_manager, tracer_manager, sat_vapor_pres, and time_manager from real to class(*)
rem1776 added a commit that referenced this pull request Feb 3, 2022
* Change send_data interface and any necessary routines for mixed real precision support
* Adds an r4 size constants file

BREAKING CHANGE: changes some argument types in diag_manager, tracer_manager, sat_vapor_pres, and time_manager from real to class(*)

Co-authored-by: Minsuk Ji <57227195+MinsukJi-NOAA@users.noreply.github.com>
rem1776 added a commit that referenced this pull request Feb 14, 2022
rem1776 added a commit that referenced this pull request Feb 17, 2022
uramirez8707 added a commit to uramirez8707/FMS that referenced this pull request Feb 24, 2022
MinsukJi-NOAA added a commit to NOAA-EMC/FMS that referenced this pull request Apr 15, 2022
* get_unit warning now only prints on root_pe (NOAA-GFDL#872)

Co-authored-by: Eric Stofferahn <Eric.Stofferahn@noaa.gov>

* Update changelog, version numbers and build info (NOAA-GFDL#873)

Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>

* chore: Change version number to next development version (NOAA-GFDL#874)

* CI: Update build action for yaml parser (NOAA-GFDL#871)

Update build actions
Change images to organization's repo and fix mixed mode parser test failure

* test(parser): Change real comparison value to double (NOAA-GFDL#886)

Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>

* Make 'unsupported kind' error messages more descriptive. Change author of constants4.F90

* Add doxygen comment to valid_types in sat_vapor_pres/sat_vapor_pres_k.F90

* Modify codes for r8-r4 conversion to remove compiler warnings

* Remove OVERLOAD_R8 directives regarding send_data_*d_r8 subroutines

* Add Doxygen comments to constants4.F90

* Updates AM4 regression test suite to run intel 21 only on PW (NOAA-GFDL#893)

* feat: emc changes for mixedmode (NOAA-GFDL#857) (NOAA-GFDL#898)

* Change send_data interface and any necessary routines for mixed real precision support
* Adds an r4 size constants file

BREAKING CHANGE: changes some argument types in diag_manager, tracer_manager, sat_vapor_pres, and time_manager from real to class(*)

Co-authored-by: Minsuk Ji <57227195+MinsukJi-NOAA@users.noreply.github.com>

* feat: adds option to override (ignore) checksum test when reading restarts (NOAA-GFDL#889)

* update to fms2_io to allow reads to ignore embedded checksums on a fileobj basis
* added fms2_io test cases for the ignore_checksum option to restart reads
tests the domain, domain_wrap, and bc restart options

* docs: add CI information file (NOAA-GFDL#899)

* test: Adds check_nml_error after reading a namelist, so it could crash only as expected (NOAA-GFDL#904)

* Revert "feat: emc changes for mixedmode (NOAA-GFDL#857) (NOAA-GFDL#898)" (NOAA-GFDL#914)

This reverts commit 516a5ef.

* fix: clean up unused/uninitialized variables and other warnings (NOAA-GFDL#859)

Remove unused variables throughout and changes/removals for other warnings such as uninitialized values and implicit casts

* docs: update function style and branch names

* Changes master to main in CONTRIBUTING.md

* Claifies function return documentation in CODE_STYLE.md

* feat: adds support for logical variables in the yaml parser (NOAA-GFDL#907)

* Adds support for logical variables in the yaml parser

* correctly determines if a string is true or false

* fix: add check for affinity to self-initialize and update test program (NOAA-GFDL#917)

* fix: adds casts to class(*) calls to match doubles in C routines (NOAA-GFDL#920)

Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>

* build: add intel code coverage build option to autotools (NOAA-GFDL#895)

* refactor: change to inclusive variable names (NOAA-GFDL#926)

* feat: Allow maximum number of restart variables to be set at build time (NOAA-GFDL#909)

* fix: Fixes for linter action and code style (NOAA-GFDL#869)

* test: Test script updates and input tests (NOAA-GFDL#800)

Rewrites test script to use added testing shell functions
Adds previously skipped or removed unit tests and support for testing with input files

Co-authored-by: Seth Underwood <Seth.Underwood@noaa.gov>
Co-authored-by: Uriel Ramirez <uriel.ramirez@noaa.gov>

* chore: 2022.01 release changes (NOAA-GFDL#941)

* chore: change version number to next development version (NOAA-GFDL#945)

* Add option for position independent code (NOAA-GFDL#930)

* fix: document and change parameter names for grid versions (NOAA-GFDL#918)

* fix: Moving empty and non-existent data table handling inside data_override (NOAA-GFDL#933)

Enables the processing of an empty or non-existent data_table or data_table.yaml from within data_override, so that any routine can call data_override_init without worry

* fix: Removal of internal FMS use of fms_io and mpp_io (NOAA-GFDL#928)

* fix: mpp changes to solve compile issues with serial builds (NOAA-GFDL#949)

* feat: adds build option for compiling with different sets of constants (NOAA-GFDL#929)

* feat: add and aggregate mpp_chksum unit tests (NOAA-GFDL#946)

* feat: add module for string utility routines (NOAA-GFDL#911)

Adds module and accompanying test for common string operations

* revert: reinstated the mpp_io routines and put them at the end for future elimination. (NOAA-GFDL#952)

* fix: add back in small_fac parameter to constants .h files (NOAA-GFDL#954)

* fix: root pe bug with fms2_io::flush_file (NOAA-GFDL#958)

* fix: clean up string routines for fms_string_utils_mod (NOAA-GFDL#953)

* Make changes to lines with more than 120 columns

Co-authored-by: Ryan Mulhall <35538242+rem1776@users.noreply.github.com>
Co-authored-by: Eric Stofferahn <Eric.Stofferahn@noaa.gov>
Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>
Co-authored-by: Tom Robinson <33458882+thomas-robinson@users.noreply.github.com>
Co-authored-by: Rusty Benson <6594772+bensonr@users.noreply.github.com>
Co-authored-by: uramirez8707 <49168881+uramirez8707@users.noreply.github.com>
Co-authored-by: Raffaele Montuoro <raffaele.montuoro@outlook.com>
Co-authored-by: Seth Underwood <Seth.Underwood@noaa.gov>
Co-authored-by: Uriel Ramirez <uriel.ramirez@noaa.gov>
Co-authored-by: Matthew Thompson <matthew.thompson@nasa.gov>
Co-authored-by: Eric <7784797+GFDL-Eric@users.noreply.github.com>
MinsukJi-NOAA added a commit to NOAA-EMC/FMS that referenced this pull request Apr 15, 2022
* get_unit warning now only prints on root_pe (NOAA-GFDL#872)

Co-authored-by: Eric Stofferahn <Eric.Stofferahn@noaa.gov>

* Update changelog, version numbers and build info (NOAA-GFDL#873)

Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>

* chore: Change version number to next development version (NOAA-GFDL#874)

* CI: Update build action for yaml parser (NOAA-GFDL#871)

Update build actions
Change images to organization's repo and fix mixed mode parser test failure

* test(parser): Change real comparison value to double (NOAA-GFDL#886)

Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>

* Make 'unsupported kind' error messages more descriptive. Change author of constants4.F90

* Add doxygen comment to valid_types in sat_vapor_pres/sat_vapor_pres_k.F90

* Modify codes for r8-r4 conversion to remove compiler warnings

* Remove OVERLOAD_R8 directives regarding send_data_*d_r8 subroutines

* Add Doxygen comments to constants4.F90

* Updates AM4 regression test suite to run intel 21 only on PW (NOAA-GFDL#893)

* feat: emc changes for mixedmode (NOAA-GFDL#857) (NOAA-GFDL#898)

* Change send_data interface and any necessary routines for mixed real precision support
* Adds an r4 size constants file

BREAKING CHANGE: changes some argument types in diag_manager, tracer_manager, sat_vapor_pres, and time_manager from real to class(*)

Co-authored-by: Minsuk Ji <57227195+MinsukJi-NOAA@users.noreply.github.com>

* feat: adds option to override (ignore) checksum test when reading restarts (NOAA-GFDL#889)

* update to fms2_io to allow reads to ignore embedded checksums on a fileobj basis
* added fms2_io test cases for the ignore_checksum option to restart reads
tests the domain, domain_wrap, and bc restart options

* docs: add CI information file (NOAA-GFDL#899)

* test: Adds check_nml_error after reading a namelist, so it could crash only as expected (NOAA-GFDL#904)

* Revert "feat: emc changes for mixedmode (NOAA-GFDL#857) (NOAA-GFDL#898)" (NOAA-GFDL#914)

This reverts commit 516a5ef.

* fix: clean up unused/uninitialized variables and other warnings (NOAA-GFDL#859)

Remove unused variables throughout and changes/removals for other warnings such as uninitialized values and implicit casts

* docs: update function style and branch names

* Changes master to main in CONTRIBUTING.md

* Claifies function return documentation in CODE_STYLE.md

* feat: adds support for logical variables in the yaml parser (NOAA-GFDL#907)

* Adds support for logical variables in the yaml parser

* correctly determines if a string is true or false

* fix: add check for affinity to self-initialize and update test program (NOAA-GFDL#917)

* fix: adds casts to class(*) calls to match doubles in C routines (NOAA-GFDL#920)

Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>

* build: add intel code coverage build option to autotools (NOAA-GFDL#895)

* refactor: change to inclusive variable names (NOAA-GFDL#926)

* feat: Allow maximum number of restart variables to be set at build time (NOAA-GFDL#909)

* fix: Fixes for linter action and code style (NOAA-GFDL#869)

* test: Test script updates and input tests (NOAA-GFDL#800)

Rewrites test script to use added testing shell functions
Adds previously skipped or removed unit tests and support for testing with input files

Co-authored-by: Seth Underwood <Seth.Underwood@noaa.gov>
Co-authored-by: Uriel Ramirez <uriel.ramirez@noaa.gov>

* chore: 2022.01 release changes (NOAA-GFDL#941)

* chore: change version number to next development version (NOAA-GFDL#945)

* Add option for position independent code (NOAA-GFDL#930)

* fix: document and change parameter names for grid versions (NOAA-GFDL#918)

* fix: Moving empty and non-existent data table handling inside data_override (NOAA-GFDL#933)

Enables the processing of an empty or non-existent data_table or data_table.yaml from within data_override, so that any routine can call data_override_init without worry

* fix: Removal of internal FMS use of fms_io and mpp_io (NOAA-GFDL#928)

* fix: mpp changes to solve compile issues with serial builds (NOAA-GFDL#949)

* feat: adds build option for compiling with different sets of constants (NOAA-GFDL#929)

* feat: add and aggregate mpp_chksum unit tests (NOAA-GFDL#946)

* feat: add module for string utility routines (NOAA-GFDL#911)

Adds module and accompanying test for common string operations

* revert: reinstated the mpp_io routines and put them at the end for future elimination. (NOAA-GFDL#952)

* fix: add back in small_fac parameter to constants .h files (NOAA-GFDL#954)

* fix: root pe bug with fms2_io::flush_file (NOAA-GFDL#958)

* fix: clean up string routines for fms_string_utils_mod (NOAA-GFDL#953)

* chore: update libFMS module with new routines (NOAA-GFDL#912)

* fix: removes fms_c.c and fms_c.h (NOAA-GFDL#961)

* Make changes to lines with more than 120 columns

Co-authored-by: Ryan Mulhall <35538242+rem1776@users.noreply.github.com>
Co-authored-by: Eric Stofferahn <Eric.Stofferahn@noaa.gov>
Co-authored-by: rem1776 <Ryan.Mulhall@lscamd50-d.gfdl.noaa.gov>
Co-authored-by: Tom Robinson <33458882+thomas-robinson@users.noreply.github.com>
Co-authored-by: Rusty Benson <6594772+bensonr@users.noreply.github.com>
Co-authored-by: uramirez8707 <49168881+uramirez8707@users.noreply.github.com>
Co-authored-by: Raffaele Montuoro <raffaele.montuoro@outlook.com>
Co-authored-by: Seth Underwood <Seth.Underwood@noaa.gov>
Co-authored-by: Uriel Ramirez <uriel.ramirez@noaa.gov>
Co-authored-by: Matthew Thompson <matthew.thompson@nasa.gov>
Co-authored-by: Eric <7784797+GFDL-Eric@users.noreply.github.com>
@MinsukJi-NOAA MinsukJi-NOAA mentioned this pull request Apr 15, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants