Update implementation of some optional arguments in Icepack #429

apcraig · 2023-02-14T20:06:51Z

PR checklist

Remove local copies where possible

Check optional arguments

Remove public interface declarations where not needed

Clean up some intent statements

- Remove local copies where possible - Check optional arguments Remove public interface declarations where not needed Clean up some intent statements

apcraig · 2023-02-14T20:08:51Z

I think I've reviewed all the current optional arguments in Icepack now and checked implementation. There are probably other Icepack arguments that could be made optional, but we'll need to come up with some suggestions if we want to do that.

I will merge this to the E3SM Icepack branch after it's merged and update CICE as well.

phil-blain

Hi Tony,

Thanks a lot for these cleanups, these are very nice and make a lot of sense (and it's also safer, see the commit message of the commit I link to below). I left a few notes/questions below but mostly this is a direction I'm happy to see the code move towards.

Just a comment on the commit message, you mention:

Clean up some intent statements

If I did not read too fast, that is in columnphysics/icepack_therm_bl99.F90 and you are just moving the intent attributes on the same line as the argument they refer to, the actual intent (in, out or inout) does not change. I would make that clear in the commit message.

phil-blain · 2023-02-14T20:51:10Z

columnphysics/icepack_fsd.F90

+      if (present(write_diags)) then
+      if (write_diags) then


This change, along with the deletions above, change the behaviour of the subroutine if write_diags is absent. Before, in that case we would write diagnostics since l_write_diags was initialized to .true. . Now we check if the argument is present, and so callers must always set write_diags to true if they want diagnostics.

I checked we already pass this argument in CICE drivers, bu we do not do so in the Icepack driver:

Icepack/configuration/driver/icedrv_InitMod.F90

Lines 100 to 105 in 67c8741

call icepack_init_fsd_bounds( &

nfsd=nfsd, & ! floe size distribution

floe_rad_l=floe_rad_l, & ! fsd size lower bound in m (radius)

floe_rad_c=floe_rad_c, & ! fsd size bin centre in m (radius)

floe_binwidth=floe_binwidth, & ! fsd size bin width in m (radius)

c_fsd_range=c_fsd_range) ! string for history output

I think this should be pointed out in the commit message, and maybe in the release notes ?

You are correct, thanks for noticing this @phil-blain. I missed that and assume it behaved like everything else in the code, didn't look closely enough. Overall, the diagnostics should either be on all the time OR they should be turned on via the interface. It doesn't make sense to me that they be on by default but can be turned OFF by the interface. That's counter to how everything works in CICE/Icepack in general. What do others think? For now, I think I'll update the call to icepack_init_fsd_bounds and set the flag to true.

phil-blain · 2023-02-14T20:57:16Z

columnphysics/icepack_itd.F90

+      if (present(fiso_ocn)) then
+         if (tr_iso) then
+            do it = 1, n_iso
+              fiso_ocn(it) = fiso_ocn(it) + dfiso_ocn(it)
+            enddo
+         endif


OK, this change is in a different category, no ? here we are hardening the code since before we were using fiso_ocn without checking that it was present, instead assuming tr_iso would be true in that case.

This is a good improvement.

phil-blain · 2023-02-14T21:03:30Z

columnphysics/icepack_orbital.F90

+      if (icepack_chkoptargflag(first_call)) then
+         if (.not.(present(days_per_year) .and. &
+                   present(nextsw_cday)   .and. &
+                   present(calendar_type))) then
+            call icepack_warnings_add(subname//' error in CESMCOUPLED args')
+            call icepack_warnings_setabort(.true.,__FILE__,__LINE__)
+            return
+         endif
+      endif
+


OK, so if I understand correctly these flags are not optional for CESM, so we add a check to that effect here?

If so, that makes sense.

This might complicate coupling in E3SM, if it's using CESMCOUPLED (I'm not sure). @njeffery

Right, there are very few CESMCOUPLED cpps left in Icepack, we tried to clear them out a while ago and shift more to namelist settings. Use of CESMCOUPLED in icepack_orbital was discussed of that at the time. If this causes E3SM a problem, we can refactor it. The problem is that it will probably require a change in all the models using CESMCOUPLED in terms of how the orbital stuff is handled, so we're trying not to change that until we have to. But maybe we should do this at some point with consultation with CESM, RASM, etc so it's well coordinated. New implementation still TBD as well.

phil-blain · 2023-02-14T21:09:17Z

columnphysics/icepack_shortwave.F90

+                               fswthru_vdr=l_fswthru_vdr,&
+                               fswthru_vdf=l_fswthru_vdf,&
+                               fswthru_idr=l_fswthru_idr,&
+                               fswthru_idf=l_fswthru_idf,&


OK, here we are in the call to absorbed_solar (not enough context is shown in this diff), and in that subroutine fswthru_vdr and friends are not optional, so here we have to keep the local counterparts.

It makes sense to makes these scalars instead, nice cleanup.

Should fswthru_vdr and friends be optional in absorbed_solar?

absorbed_solar is a local routine, it's not public. Also, it operates on 1 category while the fswthru_vdr optional arguments are arrays of size ncat. So we cannot actually pass the full optional argument thru. So this means we have to create a local copy for that scalar and copy in/out of the optional arguments. We could do various things to change this like having absorbed solar operate on all categories at once. But for now, I just left the basic implementation as it was and adapted. Those fswthru_vdr are always passed as currently implemented so adding optional wouldn't have an impact. What we'd need to do in that case is add a new flag to say whether to compute those or not based upon the arguments passed into shortwave_ccsm3. That won't affect results or performance. If absorbed_solar becomes public, then we may have to rethink this.

phil-blain · 2023-02-14T21:25:00Z

columnphysics/icepack_shortwave.F90

+         if(present(fswthru_vdr)) fswthru_vdr(n) = l_fswthru_vdr
+         if(present(fswthru_vdf)) fswthru_vdf(n) = l_fswthru_vdf
+         if(present(fswthru_idr)) fswthru_idr(n) = l_fswthru_idr
+         if(present(fswthru_idf)) fswthru_idf(n) = l_fswthru_idf


This is a very nice cleanup because these statements are inside the condition on aicen. So this means that, if for some category the condition is not matched, whatever values were in fswthru_idf and friends stay there untouched. This was not the case before, since the whole arrays were overwritten by their local counterparts, which themselves were uninitialized. This lead to a crash in one of our configurations (when compiling with NaN initialization) which lead me to carry this commit in our internal fork: phil-blain@c477637

Nice to know I'll be able to drop this one eventually :)

phil-blain · 2023-02-14T21:45:07Z

columnphysics/icepack_therm_vertical.F90

      l_meltsliq  = c0
+      l_meltsliqn = c0


OK, this was already initialized below at:

Icepack/columnphysics/icepack_therm_vertical.F90

Lines 2548 to 2555 in 67c8741

do n = 1, ncat

meltsn (n) = c0

melttn (n) = c0

meltbn (n) = c0

congeln(n) = c0

snoicen(n) = c0

dsnown (n) = c0

l_meltsliqn(n) = c0

but I think it makes sense to initialize it here, closer to the beginning of the subroutine. Maybe we'd want to remove that other initialization then ?

Actually, this is odd. In both the prior and current implementation,

l_meltsliqn = c0 if (present(meltsliqn)) l_meltsliqn = meltsliqn

then later (line 2555 above)

l_meltsliqn(n) = c0

so I see no reason to copy meltsliqn into l_meltsliqn if it's ultimately intialized to c0. The question is whether resetting l_meltsliqn(n) to c0 is a bug AFTER copying in meltsliqn. Either l_meltsliqn is initialized c0 or meltsliqn. Right now, it's confusing. @eclare108213, any thoughts? I may have messed this up in refactoring at some point, we should clarify and clean up.

@apcraig : Looks to me like this first passing of meltsliqn to l_meltsliqn is not needed. The term l_meltsliqn should be zeroed out once before thermo_vertical. Line 2555 is where it's done in mpas

Thanks @njeffery, I'll clean that up.

phil-blain · 2023-02-14T21:52:55Z

columnphysics/icepack_shortwave.F90

-      public :: run_dEdd, &
-                shortwave_ccsm3, &
-                compute_shortwave_trcr, &
-                icepack_prep_radiation, &


were the interfaces of these subroutines documented via the autodocumentation script (I did not check) ? if so, ww would want to re-run it to remove them from the public interface doc (and the same for the other subroutines that are made private in other modules).

Only the public interfaces in icepack_intfc are auto-documented. I'll confirm that and also rerun the documentation.

phil-blain · 2023-02-14T21:54:09Z

columnphysics/icepack_tracers.F90

@@ -907,7 +907,7 @@ end subroutine icepack_query_tracer_indices

      subroutine icepack_write_tracer_indices(iounit)

-        integer, intent(in), optional :: iounit


here icepack_write_tracer_indices is a public subroutine so we should re-run the autodocumentation script to update its interface in the doc to reflect the change of iounit from optional to mandatory.

Update documentation

apcraig · 2023-02-20T05:32:13Z

I think I've updated the changes requested. I retested full test suite on cheyenne with Icepack and CICE, everything passes and is bit-for-bit. This is ready for another review/merge.

eclare108213

Nice cleanup, thanks. Just a couple of comments below, and I'm flagging @njeffery and @darincomeau because there are changes here to icepack_mechred, snow and orbital that would need to be considered when coupling into E3SM.

eclare108213 · 2023-02-20T15:17:44Z

columnphysics/icepack_orbital.F90

+      if (icepack_chkoptargflag(first_call)) then
+         if (.not.(present(days_per_year) .and. &
+                   present(nextsw_cday)   .and. &
+                   present(calendar_type))) then
+            call icepack_warnings_add(subname//' error in CESMCOUPLED args')
+            call icepack_warnings_setabort(.true.,__FILE__,__LINE__)
+            return
+         endif
+      endif
+


This might complicate coupling in E3SM, if it's using CESMCOUPLED (I'm not sure). @njeffery

eclare108213 · 2023-02-20T15:20:22Z

columnphysics/icepack_shortwave.F90

+                               fswthru_vdr=l_fswthru_vdr,&
+                               fswthru_vdf=l_fswthru_vdf,&
+                               fswthru_idr=l_fswthru_idr,&
+                               fswthru_idf=l_fswthru_idf,&


Should fswthru_vdr and friends be optional in absorbed_solar?

eclare108213

This looks good to me. I'll approve now (because I'll be out the rest of the week) but I'd like @njeffery to take a look also. @phil-blain does this satisfy your comments?

phil-blain

Looks good to me!

Update implementation of some optional arguments in Icepack

67c8741

- Remove local copies where possible - Check optional arguments Remove public interface declarations where not needed Clean up some intent statements

apcraig requested review from eclare108213, dabail10 and phil-blain February 14, 2023 20:06

phil-blain reviewed Feb 14, 2023

View reviewed changes

apcraig added Driver/Interface Software Engineering labels Feb 15, 2023

apcraig added 2 commits February 15, 2023 12:24

Add write_diags argument to icepack_init_fsd_bounds

e607599

Update documentation

Clean up meltsliq implementation in icepack_step_therm1

4c9d45a

eclare108213 reviewed Feb 20, 2023

View reviewed changes

eclare108213 approved these changes Feb 20, 2023

View reviewed changes

phil-blain approved these changes Feb 21, 2023

View reviewed changes

apcraig merged commit acfc046 into CICE-Consortium:main Mar 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update implementation of some optional arguments in Icepack #429

Update implementation of some optional arguments in Icepack #429

apcraig commented Feb 14, 2023

apcraig commented Feb 14, 2023

phil-blain left a comment

phil-blain Feb 14, 2023

apcraig Feb 15, 2023

phil-blain Feb 14, 2023

phil-blain Feb 14, 2023

eclare108213 Feb 20, 2023

apcraig Feb 20, 2023

phil-blain Feb 14, 2023

eclare108213 Feb 20, 2023

apcraig Feb 20, 2023

phil-blain Feb 14, 2023

phil-blain Feb 14, 2023

apcraig Feb 15, 2023

njeffery Feb 15, 2023

apcraig Feb 15, 2023

phil-blain Feb 14, 2023

apcraig Feb 15, 2023

phil-blain Feb 14, 2023

apcraig commented Feb 20, 2023

eclare108213 left a comment

eclare108213 Feb 20, 2023

eclare108213 Feb 20, 2023

eclare108213 left a comment

phil-blain left a comment

	call icepack_init_fsd_bounds( &
	nfsd=nfsd, & ! floe size distribution
	floe_rad_l=floe_rad_l, & ! fsd size lower bound in m (radius)
	floe_rad_c=floe_rad_c, & ! fsd size bin centre in m (radius)
	floe_binwidth=floe_binwidth, & ! fsd size bin width in m (radius)
	c_fsd_range=c_fsd_range) ! string for history output

	do n = 1, ncat
	meltsn (n) = c0
	melttn (n) = c0
	meltbn (n) = c0
	congeln(n) = c0
	snoicen(n) = c0
	dsnown (n) = c0
	l_meltsliqn(n) = c0

		@@ -907,7 +907,7 @@ end subroutine icepack_query_tracer_indices

		subroutine icepack_write_tracer_indices(iounit)

		integer, intent(in), optional :: iounit

Update implementation of some optional arguments in Icepack #429

Update implementation of some optional arguments in Icepack #429

Conversation

apcraig commented Feb 14, 2023

PR checklist

apcraig commented Feb 14, 2023

phil-blain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apcraig commented Feb 20, 2023

eclare108213 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclare108213 left a comment

Choose a reason for hiding this comment

phil-blain left a comment

Choose a reason for hiding this comment