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.

Sign up for GitHub

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

Jump to bottom

Update CPP implementation #490

Merged
merged 19 commits into from
Jul 31, 2020
Merged

Update CPP implementation #490

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
bc3895c
Update CPPs
apcraig Jul 15, 2020
a3328a6
Convert ORCA_GRID CPP to orca_halogrid namelist
apcraig Jul 17, 2020
4a14f9f
update documentation
apcraig Jul 17, 2020
5d2abae
update documentation
apcraig Jul 17, 2020
355b10a
update documentation
apcraig Jul 17, 2020
b315b1f
update documentation
apcraig Jul 17, 2020
80260ee
update documentation
apcraig Jul 17, 2020
d5089e5
update documentation
apcraig Jul 17, 2020
d01bc3a
update documentation
apcraig Jul 17, 2020
1f6ce89
update documentation
apcraig Jul 17, 2020
0324512
update documentation
apcraig Jul 23, 2020
2d59c5c
Update doc/source/cice_index.rst
apcraig Jul 29, 2020
42d06bc
Update doc/source/user_guide/ug_case_settings.rst
apcraig Jul 29, 2020
4362a00
Update doc/source/user_guide/ug_case_settings.rst
apcraig Jul 29, 2020
c45569d
Update doc/source/user_guide/ug_running.rst
apcraig Jul 29, 2020
fc640ad
Update doc/source/user_guide/ug_running.rst
apcraig Jul 29, 2020
72bd116
Update doc/source/user_guide/ug_running.rst
apcraig Jul 29, 2020
7976305
Update doc/source/user_guide/ug_running.rst
apcraig Jul 29, 2020
f7ce46e
update documentation, remove coupled macros as this is going away soo…
apcraig Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 26 additions & 65 deletions cicecore/cicedynB/general/ice_forcing.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#ifdef ncdf
#define USE_NETCDF
#endif
!=======================================================================
!
! Reads and interpolates forcing data for atmosphere and ocean quantities.
Expand Down Expand Up @@ -300,9 +303,6 @@ subroutine init_forcing_ocn(dt)
use ice_domain, only: nblocks
use ice_domain_size, only: max_blocks
use ice_flux, only: sss, sst, Tf
#ifdef ncdf
use netcdf
#endif

real (kind=dbl_kind), intent(in) :: &
dt ! time step
Expand Down Expand Up @@ -866,7 +866,6 @@ subroutine read_data_nc (flag, recd, yr, ixm, ixx, ixp, &

character(len=*), parameter :: subname = '(read_data_nc)'

#ifdef ncdf
integer (kind=int_kind) :: &
nrec , & ! record number to read
n2, n4 , & ! like ixm and ixp, but
Expand Down Expand Up @@ -967,9 +966,6 @@ subroutine read_data_nc (flag, recd, yr, ixm, ixx, ixp, &

call ice_timer_stop(timer_readwrite) ! reading/writing

#else
field_data = c0 ! to satisfy intent(out) attribute
#endif
Comment on lines -970 to -972
Copy link
Member

Choose a reason for hiding this comment

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

I guess this code is deleted because it should never be reached ? is that the logic ? we should never call read_data_nc if USE_NETCDF is not active ? (I'm not familiar yet with this part of the code, sorry if I'm asking questions that seem evident).

Copy link
Contributor

Choose a reason for hiding this comment

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

In principle it should not reach this part, however if it for whatever reason reaches this part anyway then it should fail in the call to ice_open_nc as the netcdf file cannot be read. Here should be an abort and a cpp flag.

read_data_nc do not use the netcdf library therefore it should be able to run through without warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

If read_data_nc doesn't use the netcdf library at all then it shouldn't have the _nc suffix on the subroutine name. I think this routine has evolved over time.

The field_data=c0 line was necessary years ago because a compiler (I don't remember which one, most likely intel) wouldn't make it through the build without it, when the cpp was not invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not need the protection of the USE_NETCDF or the #else here. The code will build fine, there are no calls directly to netcdf in this subroutine. What it does is call _nc routines which will abort if USE_NETCDF is not set. So anytime this routine is called when USE_NETCDF is set, it should work. Anytime this routine is called when USE_NETCDF is not set, it will abort in a lower level subroutine. I think that's the right behavior.

end subroutine read_data_nc

!=======================================================================
Expand Down Expand Up @@ -1007,7 +1003,6 @@ subroutine read_data_nc_hycom (flag, recd, &
intent(out) :: &
field_data ! 2 values needed for interpolation

#ifdef ncdf
! local variables
integer (kind=int_kind) :: &
fid ! file id for netCDF routines
Expand Down Expand Up @@ -1040,11 +1035,6 @@ subroutine read_data_nc_hycom (flag, recd, &

call ice_timer_stop(timer_readwrite) ! reading/writing

#else
field_data = c0 ! to satisfy intent(out) attribute
write(*,*)'ERROR: CICE not compiled with NetCDF'
stop
#endif
end subroutine read_data_nc_hycom

!=======================================================================
Expand Down Expand Up @@ -3342,9 +3332,6 @@ subroutine oned_data

use ice_flux, only: uatm, vatm, Tair, fsw, fsnow, Qa, rhoa, frain

#ifdef ncdf
use netcdf

! local parameters

character (char_len_long) :: &
Expand Down Expand Up @@ -3402,7 +3389,7 @@ subroutine oned_data
Temp = work
Tair(:,:,:) = Temp

if (my_task == master_task) status = nf90_close(fid)
call ice_close_nc(fid)

! hourly solar data beginning Jan 1, 1989, 01:00
met_file = fsw_file
Expand All @@ -3412,7 +3399,7 @@ subroutine oned_data
call ice_read_nc(fid,istep1,fieldname,work,diag)
fsw(:,:,:) = work

if (my_task == master_task) status = nf90_close(fid)
call ice_close_nc(fid)

! hourly interpolated monthly data beginning Jan 1, 1989, 01:00
met_file = humid_file
Expand All @@ -3426,7 +3413,7 @@ subroutine oned_data
call ice_read_nc(fid,istep1,fieldname,work,diag)
fsnow(:,:,:) = work

if (my_task == master_task) status = nf90_close(fid)
call ice_close_nc(fid)

!-------------------------------------------------------------------
! Find specific humidity using Hyland-Wexler formulation
Expand All @@ -3447,8 +3434,6 @@ subroutine oned_data
cldf (:,:,:) = p25 ! cloud fraction
frain(:,:,:) = c0 ! this is available in hourlymet_rh file

#endif

end subroutine oned_data

!=======================================================================
Expand Down Expand Up @@ -3648,7 +3633,7 @@ subroutine ocn_data_ncar_init

use ice_blocks, only: nx_block, ny_block
use ice_domain_size, only: max_blocks
#ifdef ncdf
#ifdef USE_NETCDF
use netcdf
#endif

Expand All @@ -3664,7 +3649,6 @@ subroutine ocn_data_ncar_init
'T', 'S', 'hblt', 'U', 'V', &
'dhdx', 'dhdy', 'qdp' /

#ifdef ncdf
Copy link
Member

Choose a reason for hiding this comment

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

Why was this one not converted to USE_NETCDF ? this will cause warnings if we eventually clean up the code so that it compiles without warnings with -Wall (which I think we should do eventually...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it is ok to remove it but at the same time on line 3704 and onwards there are calls to nf90_inq_dimid and nf90_inquire_dimension. I think that these should be moved to ice_open_nc as optional parameters. Then the surrounding USE_NETCDF should also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ifdef on line 3667 is not needed. The check at line 3704 takes care of it. I'm not sure I see a problem in the section of code between 3666 and 3685 that would create a problem for -Wall (and I agree we should work towards that capability). The code refactoring here was for the cpps, not netcdf. I agree there are many things that could be done to improve the netcdf implementation or even the io implementation overall (like merging the io_binary, io_netcdf, and io_pio and make them run-time settable on a per file type basis). At this point, I think a netcdf cleanup should be done separately. But I agree with the strategy that @TillRasmussen has outlined in terms of improving the code.

Copy link
Member

Choose a reason for hiding this comment

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

My point was that if we remove the CPP around the variable declarations, and compile with -Wall and without USE_NETCDF, the compiler will complain that these variables are declared but unused. It's a minor issue but I thought I'd point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, good point.

integer (kind=int_kind) :: &
fid , & ! file id
dimid ! dimension id
Expand All @@ -3673,7 +3657,6 @@ subroutine ocn_data_ncar_init
status , & ! status flag
nlat , & ! number of longitudes of data
nlon ! number of latitudes of data
#endif

real (kind=dbl_kind), dimension (nx_block,ny_block,max_blocks) :: &
work1
Expand Down Expand Up @@ -3701,7 +3684,7 @@ subroutine ocn_data_ncar_init
endif ! master_task

if (trim(ocn_data_format) == 'nc') then
#ifdef ncdf
#ifdef USE_NETCDF
if (my_task == master_task) then
call ice_open_nc(sst_file, fid)

Expand Down Expand Up @@ -3741,7 +3724,10 @@ subroutine ocn_data_ncar_init
enddo ! month loop
enddo ! field loop

if (my_task == master_task) status = nf90_close(fid)
if (my_task == master_task) call ice_close_nc(fid)
Copy link
Member

Choose a reason for hiding this comment

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

The if (my_task == master_task) check seems redundant, since it's done in ice_close_nc, no?

Copy link
Contributor

@TillRasmussen TillRasmussen Jul 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes (as in I agree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The master_task if is redundant. I did not want to change the current implementation too much so I left it as is. The redundancy is not a problem. I agree this could be cleaned up. But once you start down that path, there is a lot to do. Just look at the whole ocn_data_ncar_init routine as one example. My goal here was to change the nf90_close to a call to ice_close_nc because it was easy to do and then matches the ice_open_nc above.

#else
call abort_ice(subname//'ERROR: USE_NETCDF cpp not defined for '//trim(sst_file), &
file=__FILE__, line=__LINE__)
#endif

else ! binary format
Expand Down Expand Up @@ -3803,11 +3789,11 @@ subroutine ocn_data_ncar_init_3D
use ice_domain_size, only: max_blocks
use ice_grid, only: to_ugrid, ANGLET
use ice_read_write, only: ice_read_nc_uv
#ifdef ncdf
#ifdef USE_NETCDF
use netcdf
#endif

#ifdef ncdf
#ifdef USE_NETCDF
integer (kind=int_kind) :: &
n , & ! field index
m , & ! month index
Expand Down Expand Up @@ -3856,7 +3842,7 @@ subroutine ocn_data_ncar_init_3D
endif ! master_task

if (trim(ocn_data_format) == 'nc') then
#ifdef ncdf
#ifdef USE_NETCDF
if (my_task == master_task) then
call ice_open_nc(sst_file, fid)

Expand Down Expand Up @@ -3902,7 +3888,7 @@ subroutine ocn_data_ncar_init_3D
enddo ! month loop
enddo ! field loop

if (my_task == master_task) status = nf90_close(fid)
if (my_task == master_task) call ice_close_nc(fid)
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here.


! Rotate vector quantities and shift to U-grid
do n=4,6,2
Expand All @@ -3923,6 +3909,9 @@ subroutine ocn_data_ncar_init_3D
enddo ! month loop
enddo ! field loop

#else
call abort_ice(subname//'ERROR: USE_NETCDF cpp not defined', &
file=__FILE__, line=__LINE__)
#endif

else ! binary format
Expand Down Expand Up @@ -4327,9 +4316,6 @@ subroutine ocn_data_hycom_init
use ice_blocks, only: nx_block, ny_block
use ice_domain, only: nblocks
use ice_flux, only: sss, sst, Tf
#ifdef ncdf
use netcdf
#endif

integer (kind=int_kind) :: &
i, j, iblk , & ! horizontal indices
Expand Down Expand Up @@ -4611,7 +4597,6 @@ subroutine read_data_nc_point (flag, recd, yr, ixm, ixx, ixp, &

character(len=*), parameter :: subname = '(read_data_nc_point)'

#ifdef ncdf
integer (kind=int_kind) :: &
nrec , & ! record number to read
n2, n4 , & ! like ixm and ixp, but
Expand Down Expand Up @@ -4723,9 +4708,6 @@ subroutine read_data_nc_point (flag, recd, yr, ixm, ixx, ixp, &

call ice_timer_stop(timer_readwrite) ! reading/writing

#else
field_data = c0 ! to satisfy intent(out) attribute
#endif
end subroutine read_data_nc_point

!=======================================================================
Expand Down Expand Up @@ -4779,13 +4761,9 @@ subroutine ISPOL_data
!
use ice_flux, only: uatm, vatm, Tair, fsw, Qa, rhoa, &
frain, fsnow, flw
#ifdef ncdf
use netcdf
#endif

!local parameters

#ifdef ncdf
character (char_len_long) :: &
met_file, & ! netcdf filename
fieldname ! field name in netcdf file
Expand Down Expand Up @@ -4822,15 +4800,13 @@ subroutine ISPOL_data
sec1hr ! number of seconds in 1 hour

logical (kind=log_kind) :: read1
#endif

integer (kind=int_kind) :: &
recnum , & ! record number
recnum4X ! record number

character(len=*), parameter :: subname = '(ISPOL_data)'

#ifdef ncdf
call icepack_query_parameters(secday_out=secday)
call icepack_warnings_flush(nu_diag)
if (icepack_warnings_aborted()) call abort_ice(error_message=subname, &
Expand Down Expand Up @@ -4965,14 +4941,6 @@ subroutine ISPOL_data
flw(:,:,:) = c1intp * flw_data_p(1) &
+ c2intp * flw_data_p(2)
endif !nc
#else

uatm(:,:,:) = c0 !wind velocity (m/s)
vatm(:,:,:) = c0
fsw(:,:,:) = c0
fsnow (:,:,:) = c0

#endif

!flw given cldf and Tair calculated in prepare_forcing

Expand Down Expand Up @@ -5015,11 +4983,7 @@ subroutine ocn_data_ispol_init
!
use ice_gather_scatter
use ice_read_write
#ifdef ncdf
use netcdf
#endif

#ifdef ncdf
integer (kind=int_kind) :: &
n , & ! field index
m ! month index
Expand All @@ -5038,7 +5002,6 @@ subroutine ocn_data_ispol_init

integer (kind=int_kind) :: &
status ! status flag
#endif

character(len=*), parameter :: subname = '(ocn_data_ispol_init)'

Expand All @@ -5058,7 +5021,6 @@ subroutine ocn_data_ispol_init
endif ! master_task

if (trim(ocn_data_format) == 'nc') then
#ifdef ncdf
if (my_task == master_task) then
call ice_open_nc(sst_file, fid)
endif ! master_task
Expand All @@ -5078,8 +5040,7 @@ subroutine ocn_data_ispol_init
enddo ! month loop
enddo ! field loop

if (my_task == master_task) status = nf90_close(fid)
#endif
if (my_task == master_task) call ice_close_nc(fid)
Copy link
Member

Choose a reason for hiding this comment

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

And here.


else ! binary format
call abort_ice (error_message=subname//'new ocean forcing is netcdf only', &
Expand Down Expand Up @@ -5188,9 +5149,6 @@ subroutine get_wave_spec
use ice_constants, only: c0
use ice_domain_size, only: nfreq
use ice_timers, only: ice_timer_start, ice_timer_stop, timer_fsd
#ifdef ncdf
use netcdf
#endif

! local variables
integer (kind=int_kind) :: &
Expand Down Expand Up @@ -5228,16 +5186,19 @@ subroutine get_wave_spec
! read more realistic data from a file
if ((trim(wave_spec_type) == 'constant').OR.(trim(wave_spec_type) == 'random')) then
if (trim(wave_spec_file(1:4)) == 'unkn') then
call abort_ice (subname//'ERROR: wave_spec_file '//trim(wave_spec_file))
call abort_ice (subname//'ERROR: wave_spec_file '//trim(wave_spec_file), &
file=__FILE__, line=__LINE__)
else
#ifdef ncdf
#ifdef USE_NETCDF
call ice_open_nc(wave_spec_file,fid)
call ice_read_nc_xyf (fid, 1, 'efreq', wave_spectrum(:,:,:,:), dbug, &
field_loc_center, field_type_scalar)
call ice_close_nc(fid)
#else
write (nu_diag,*) "wave spectrum file not available, requires ncdf"
write (nu_diag,*) "wave spectrum file not available, requires cpp USE_NETCDF"
write (nu_diag,*) "wave spectrum file not available, using default profile"
call abort_ice (subname//'ERROR: wave_spec_file '//trim(wave_spec_file), &
Copy link
Member

Choose a reason for hiding this comment

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

Why are we aborting here when we weren't before? If we are aborting we should not output "using default profile", no?

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned earlier I think that the abort should be in the ice_open_nc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did add the abort because I thought it was the right thing to do. I think it's dangerous for a user to setup use of the wave spec file and then for it to relatively silently revert to the default values at runtime. What should happen is the user should modify their namelist so it uses the default values and does not try to read a file. That would give the same result but be explicit. Another option would be to add a binary file reading capability for when netcdf is not available, but that's probably not the way to go at this time. I would be interested in hearing what others think about this silent mode. We have shifted the overall implementation from being one that changes the user settings at runtime to fix conflicts to one that aborts when there are conflicts. I think this is a place where we want to follow that strategy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it is dangerous to revert to a default setting if the file is missing. This may happen from time to time in a operational setup due to delays of a required data flow. The abort option is better.

Copy link
Member

Choose a reason for hiding this comment

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

That indeed makes sense. This is the kind of explanations that I think we should put in commit messages :) This way we can read the commit messages and look at the changes and understand faster why these changes are made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, we should do a better of explaining changes in either our commit messages or the PR. I'll try to do better.

Copy link
Member

Choose a reason for hiding this comment

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

Just a note, I would prefer we use the commit messages; this way the information is encoded in the repo. If GitHub goes under someday for whatever reason, the commit messages stay, but the PR discussions might be more difficult to retrieve. I think I read somewhere (a very early PR?) that the GitHub projects for CICE and Icepack themselves are backed up somewhere (how regularly?), but this is not documented anywhere...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added a couple sentences to the git workflow guide to indicate we want comprehensive commit messages. I agree it's the right way to go.

file=__FILE__, line=__LINE__)
#endif
endif
endif
Expand Down
Loading