search

NCAR / DART

Data Assimilation Research Testbed
https://dart.ucar.edu/
Apache License 2.0
184 stars 139 forks source link

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

Closed mjs2369 closed 6 months ago

mjs2369 commented 7 months ago

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

Documentation changes needed?

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

Checklist for release

Testing Datasets

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

mjs2369 commented 7 months ago

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? https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L614

hkershaw-brown commented 7 months ago

Hi Marlee, do you have some test input to run this on? general paranoia about integer maths, e.g. this int < real: https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L751

mjs2369 commented 7 months ago

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

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

@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 commented 7 months ago

yup I believe so.

braczka commented 7 months ago

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?

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

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 commented 7 months ago

Hi Marlee, do you have some test input to run this on? general paranoia about integer maths, e.g. this int < real: https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L751

@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 commented 7 months ago

Hi Marlee, do you have some test input to run this on? general paranoia about integer maths, e.g. this int < real: https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L751

@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 commented 7 months ago

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? https://github.com/NCAR/DART/blob/99ebec56a6efdd243ed026da24b29a1a45bb8cdd/models/clm/dart_to_clm.f90#L614

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 commented 7 months ago

@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?

mjs2369 commented 6 months ago

@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 commented 6 months ago

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

mjs2369 commented 6 months ago

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

braczka commented 6 months ago

@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.