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

bug-fix: CCE compiler error and warnings for dart_to_clm variable snlsno #606

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

mjs2369
Copy link
Contributor

@mjs2369 mjs2369 commented Dec 19, 2023

Description:

Quickbuild.sh was erroring out and produced multiple warnings for the dart_to_clm build with the cce compiler

Building  dart_to_clm  build  12  of  12
..................................................................................... Makefile is ready.
ftn -O2  -I/glade/u/apps/derecho/23.06/spack/opt/spack/netcdf/4.9.2/cce/15.0.1/cuko/include  -c	/glade/derecho/scratch/hkershaw/build_everything/DART.cce/models/clm/dart_to_clm.f90


ftn-1569 ftn: WARNING UPDATE_SNOW, File = ../../../build_everything/DART.cce/models/clm/dart_to_clm.f90, Line = 612, Column = 19 
  A DO loop variable or expression of type default real or double precision real is a deleted feature of the Fortran standard.


ftn-1569 ftn: WARNING UPDATE_SNOW, File = ../../../build_everything/DART.cce/models/clm/dart_to_clm.f90, Line = 628, Column = 19 
  A DO loop variable or expression of type default real or double precision real is a deleted feature of the Fortran standard.


ftn-319 ftn: ERROR UPDATE_SNOW, File = ../../../build_everything/DART.cce/models/clm/dart_to_clm.f90, Line = 752, Column = 76 
  A subscript must be a scalar integer expression.

Cray Fortran : Version 15.0.1 (20230120205242_66f7391d6a03cf932f321b9f6b1d8612ef5f362c)
Cray Fortran : Compile time:  0.0513 seconds
Cray Fortran : 919 source lines
Cray Fortran : 1 errors, 2 warnings, 0 other messages, 0 ansi
Cray Fortran : "explain ftn-message number" gives more information'

The fix changes the arrays for number of snow layers (snlsno and clm_SNLSNO) from real(r8) to integer types.

These arrays being of type int is actually consistent with the description of the variable in this comment:
https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L365C1-L367C41

This change allows dart_to_clm to compile successfully and without warnings with cce.

Fixes issue

Fixes #598

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Documentation changes needed?

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.

Tests

Compiled dart_to_clm with cce
To test the execution of the program, a dart_posterior.nc file is needed.

Also built with gfortran with full debugging flags

Checklist for merging

  • Updated changelog entry
  • Documentation updated
  • Update conf.py

Checklist for release

  • Merge into main
  • Create release from the main branch with appropriate tag
  • Delete feature-branch

Testing Datasets

  • Dataset needed for testing available upon request
  • Dataset download instructions included
  • No dataset needed

Again, a dart_posterior.nc file is needed if we want to test the execution of the program.

@mjs2369 mjs2369 added the fortran standards compiler issues and (non) standard-compliant code label Dec 19, 2023
@mjs2369
Copy link
Contributor Author

mjs2369 commented Dec 19, 2023

An unrelated question I had with regards to the dart_to_clm code is that the comment below says we are forcing the existing layers to be near zero but not exactly zero:

https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L610C1-L612C35

This is true for h2oice_po and dzsno_po, but h2oliq_po is getting set to exactly zero?

h2oliq_po(ilevel,icolumn) = 0.0_r8

@hkershaw-brown
Copy link
Member

Hi Marlee, do you have some test input to run this on?
general paranoia about integer maths, e.g. this int < real:

> 0.0_r8 .and. snlsno(icolumn) < 0.0_r8) then

@mjs2369
Copy link
Contributor Author

mjs2369 commented Dec 19, 2023

Hi Marlee, do you have some test input to run this on? general paranoia about integer maths, e.g. this int < real:

> 0.0_r8 .and. snlsno(icolumn) < 0.0_r8) then

@hkershaw-brown I mention in the body of the pull request that I do not have input (dart_posterior.nc) to run dart_to_clm with. Do you have one? If not, maybe Brett does

This is a good catch. I think we will want to change this to > 0.0_r8 .and. snlsno(icolumn) < 0) then regardless of whether it runs correctly, right?

@hkershaw-brown
Copy link
Member

yup I believe so.

@braczka
Copy link
Contributor

braczka commented Dec 19, 2023

An unrelated question I had with regards to the dart_to_clm code is that the comment below says we are forcing the existing layers to be near zero but not exactly zero:

https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L610C1-L612C35

This is true for h2oice_po and dzsno_po, but h2oliq_po is getting set to exactly zero?

h2oliq_po(ilevel,icolumn) = 0.0_r8

I think you are correct to make this consistent with the other values, and make it slightly larger than zero. Additional testing has shown high sensitivity to setting values = 0, which I think we need to avoid. It is important to leave the dynamics of the layers (fission/fusion) to the CLM code snow code. Also, I suspect, other snow albedo properties are calculated from these snow layer values, and having a value of 0 tends to introduce problems.

@braczka
Copy link
Contributor

braczka commented Dec 19, 2023

Hi Marlee, do you have some test input to run this on? general paranoia about integer maths, e.g. this int < real:

> 0.0_r8 .and. snlsno(icolumn) < 0.0_r8) then

@hkershaw-brown I mention in the body of the pull request that I do not have input (dart_posterior.nc) to run dart_to_clm with. Do you have one? If not, maybe Brett does

This is a good catch. I think we will want to change this to > 0.0_r8 .and. snlsno(icolumn) < 0) then regardless of whether it runs correctly, right?

So Xueli has been testing this code and should have a posterior.nc to test. She also ran into another bug where snow layer properties could become negative during the re-parititioning and had to implement some post filter clamping to avoid this. Would be good to bring her in on the review as well.

@mjs2369
Copy link
Contributor Author

mjs2369 commented Dec 19, 2023

Hi Marlee, do you have some test input to run this on? general paranoia about integer maths, e.g. this int < real:

> 0.0_r8 .and. snlsno(icolumn) < 0.0_r8) then

@hkershaw-brown I mention in the body of the pull request that I do not have input (dart_posterior.nc) to run dart_to_clm with. Do you have one? If not, maybe Brett does
This is a good catch. I think we will want to change this to > 0.0_r8 .and. snlsno(icolumn) < 0) then regardless of whether it runs correctly, right?

So Xueli has been testing this code and should have a posterior.nc to test. She also ran into another bug where snow layer properties could become negative during the re-parititioning and had to implement some post filter clamping to avoid this. Would be good to bring her in on the review as well.

Thanks Brett. We can wait for @XueliHuo feedback and hopefully input files to test. Forgot to mention but we will also need a CLM restart file to modify (clm_restart.nc) in addition to posterior.nc

@mjs2369
Copy link
Contributor Author

mjs2369 commented Dec 19, 2023

An unrelated question I had with regards to the dart_to_clm code is that the comment below says we are forcing the existing layers to be near zero but not exactly zero:
https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L610C1-L612C35
This is true for h2oice_po and dzsno_po, but h2oliq_po is getting set to exactly zero?

h2oliq_po(ilevel,icolumn) = 0.0_r8

I think you are correct to make this consistent with the other values, and make it slightly larger than zero. Additional testing has shown high sensitivity to setting values = 0, which I think we need to avoid. It is important to leave the dynamics of the layers (fission/fusion) to the CLM code snow code. Also, I suspect, other snow albedo properties are calculated from these snow layer values, and having a value of 0 tends to introduce problems.

@hkershaw-brown would we want to go ahead and make this fix on this PR or does it warrant opening a separate issue?

@hkershaw-brown
Copy link
Member

@mjs2369 just been chatting to Brett about this -
Xueli's being running a lot of CLM-DART and has some fixes too. So how about this pull request becomes the fixup dart_to_clm pull request, we get some nice test case(s) to dart_to_clm, and multiple eyes on the code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the case I ran dart_to_clm, h2oliq_po was negative after the partitioning, causing CLM to abort, as h2oliq should never be negative.

A quick fix is adding two lines below line 688 to ensure that both h2oliq_po and h2oice_po are never negative.
if (h2oliq_po(ilevel,icolumn) < 0.0_r8) h2oliq_po(ilevel,icolumn) = 0.00000001_r8
if (h2oliq_po(ilevel,icolumn) < 0.0_r8) h2oice_po(ilevel,icolumn) = 0.00000001_r8

Same for dzsno_po and snowdp_po.
Add the line below line 697 for dzsno_po:
if (dzsno_po(ilevel,icolumn) < 0.0_r8) dzsno_po(ilevel,icolumn) = 0.00000001_r8
Add the line below line 755 for snowdp_po:
if (snowdp_po(icolumn) < 0.0_r8) snowdp_po(icolumn) = 0.00000001_r8

Alternatively, a more sophisticated approach would involve modifying the code to apply the clamping values for the above variables, as defined in clm_variables, directly in dart_to_clm.f90.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the case I ran dart_to_clm, h2oliq_po was negative after the partitioning, causing CLM to abort, as h2oliq should never be negative.

A quick fix is adding two lines below line 688 to ensure that both h2oliq_po and h2oice_po are never negative. if (h2oliq_po(ilevel,icolumn) < 0.0_r8) h2oliq_po(ilevel,icolumn) = 0.00000001_r8 if (h2oliq_po(ilevel,icolumn) < 0.0_r8) h2oice_po(ilevel,icolumn) = 0.00000001_r8

Same for dzsno_po and snowdp_po. Add the line below line 697 for dzsno_po: if (dzsno_po(ilevel,icolumn) < 0.0_r8) dzsno_po(ilevel,icolumn) = 0.00000001_r8 Add the line below line 755 for snowdp_po: if (snowdp_po(icolumn) < 0.0_r8) snowdp_po(icolumn) = 0.00000001_r8

Alternatively, a more sophisticated approach would involve modifying the code to apply the clamping values for the above variables, as defined in clm_variables, directly in dart_to_clm.f90.

Thanks @XueliHuo ! I would recommend setting these small - non zero clamping values within the dart_to_clm code only. Relying on the user to enter these high precision values in the namelist file might be confusing. A value of '0' is physically plausbile, but the very small, non-zero value is a requirement of the CLM model (a model artifact) because it dynamically generates snow layers. A general CLM-DART wouldn't be expected to know that.

Also @mjs2369 if you want to implement @XueliHuo changes directly to this PR go ahead. Alternatively you could guide her through the process of pushing her local changes to this branch. Whatever you think is best.

@mjs2369
Copy link
Contributor Author

mjs2369 commented Dec 20, 2023

@XueliHuo Thanks for the initial review! I made and pushed the changes you suggested. Do you have any test cases for dart_to_clm we could use (clm_restart.nc and posterior.nc)? It would be great if you could also run a test on your end if you have the time.

@hkershaw-brown hkershaw-brown added the release! bundle with next release label Jan 8, 2024
@hkershaw-brown
Copy link
Member

This looks like all the changes from Xueli have been made. Is this pull request good to go for the next release?

@mjs2369
Copy link
Contributor Author

mjs2369 commented Jan 8, 2024

This looks like all the changes from Xueli have been made. Is this pull request good to go for the next release?

All the changes have been made, and the code was compiled. It just hasn't been run since we do not have the necessary input files

Copy link
Member

@hkershaw-brown hkershaw-brown left a comment

Choose a reason for hiding this comment

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

The fix for loop, and array access found in issue #598 looks good: snlsno changed to integer. As noted, this variable is an integer in the netcdf file.

Clamping to negative to a very small value fix from Xueli looks to be implement ok.

Approved!

@braczka
Copy link
Contributor

braczka commented Jan 9, 2024

@hkershaw-brown, @mjs2369 -- I was off yesterday, but I should be able to look at this today/tomorrow. I want to run it through the tutorial case before including with the next release. @XueliHuo we should also run it through the Western US example case that we were discussing prior to holiday break. There may be some fine tuning we need to do on the science side to get this to work as intended. This PR could included in the next release and we could issue a separate issue for the science stuff... if needed.

@hkershaw-brown hkershaw-brown removed the release! bundle with next release label Jan 9, 2024
Copy link
Contributor

@braczka braczka left a comment

Choose a reason for hiding this comment

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

This looks good. I tested it with snow repartitioning turned on for the CLM5-DART tutorial example. I think we need to apply more rigorous tests (e.g. Western US) in the future, but these additional tests will take longer than I anticipated -- and is out of scope of the original intent of this PR. Will make additional PRs aimed at the snow repartitioning scheme itself in the future.

Correct CLM-DART sourcemod path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fortran standards compiler issues and (non) standard-compliant code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dart_to_clm CCE compiler does not build
4 participants