Refactor optional argument implementation for isotopes, snwgrain, therm1 and therm2 #423

apcraig · 2023-01-11T19:39:50Z

PR checklist

- Use argcheck design - Eliminate local copies of optional data - Pass optional arguments down the calling tree - Some minor format cleanup

apcraig · 2023-01-11T19:43:08Z

This refactors the isotope implementation using the newer design, passing optional arguments down the calling tree and using the argcheck design. If this seems reasonable, I will refactor other Icepack features in a similar way where reasonable. Do we want a PR for each feature refactor or just one (or a few) PRs to cover multiple feature refactors? Would like to get some feedback before moving forward.

This should be bit-for-bit and has no impact on the public interfaces. Will run comprehensive tests to confirm TBD.

dabail10

Sorry for not responding to the email. This looks fine to me.

eclare108213 · 2023-01-11T20:18:34Z

columnphysics/icepack_atmo.F90

-                                   uvel=l_uvel, vvel=l_vvel,  &
-                                   Uref=l_Uref, zlvs=zlvs)
+                                   Qa_iso=Qa_iso,           &
+                                   Qref_iso=Qref_iso,       &


What happens if present(Qref_iso)=F? Is this robust across all compilers/platforms? You probably tested that previously...

This is robust. The optional attribute and "present" state is passed down the calling tree. As long as optional is set in the calling routine for this field if want to check it's "present" state (determined by the original caller) and/or the optional argument is not used unless it exists, all should be fine. You can definitely get into trouble if you pass the optional argument down the calling tree and use it if it wasn't passed. Hopefully, we'll avoid that with the logic we have setup.

columnphysics/icepack_mechred.F90

eclare108213 · 2023-01-11T20:26:23Z

Do we want a PR for each feature refactor or just one (or a few) PRs to cover multiple feature refactors?

One or a few to cover multiple features would be fine, but if something is particularly complicated, then just do that by itself. E.g. I imagine that the thermodynamics will be complicated, since there are many options that feed into it.

…ocn, H2_16O_ocn, H2_18O_ocn - Modify fsloss optional argument implementation - Modify rsnwn, smicen, smliqn optional argument implementation

- Clean up some formatting

apcraig · 2023-01-14T18:17:15Z

I think it may be worth reviewing and merging this now. Most of the therm1 and therm2 optional arguments have been refactored. Still waiting on some additional insight about meltsliq, but that can be done in a separate PR.

apcraig · 2023-01-16T02:31:38Z

Icepack and CICE test suites are bit-for-bit.

phil-blain · 2023-01-16T13:55:51Z

columnphysics/icepack_atmo.F90

+      !------------------------------------------------------------
+      ! Check optional arguments
+      ! This is used for ocean and ice sfctype, need to be careful
+      !------------------------------------------------------------


I'm not sure I understand this comment, why does it say

This is used for ocean and ice sfctype

but then we just check sfctype == 'ice' ? What is "This" referring to?

I think atmo_boundary_layer is also called from the ocean mixed layer routine, in which case the surface type that the atmosphere sees is ocean instead of ice. The mixed layer routine is not used for coupled simulations, usually, and isotopes might not be implemented in a separate ocean component model, which would then require some care here.

I need to clean up this comment.

phil-blain · 2023-01-16T14:08:56Z

This refactors the isotope implementation using the newer design, passing optional arguments down the calling tree and using the argcheck design. If this seems reasonable, I will refactor other Icepack features in a similar way where reasonable.

I think this is a nice clean up, it improves readability, I think. Also, gotta love a PR that removes more lines of code than it adds ;)

Do we want a PR for each feature refactor or just one (or a few) PRs to cover multiple feature refactors? Would like to get some feedback before moving forward.

If I were doing it, I think I would probably do it that way:

1 PR per module (so 1 per file)
In each PR, one commit per subroutine: removing l_* local variable, using argcheck to validate arguments, and adjusting calls to other subroutines if need be.

This would be very much more pleasant to review, at least for me :)

Also I would avoid changing the order of the variables attributes (like moving optional to be the last attributes, with no other changes). This adds a lot of noise to the diffs. If we want to make such changes, that's OK, but I'd prefer if it were done in separate commits.

Thanks a lot for starting the work on this! :)

phil-blain · 2023-01-16T14:11:26Z

columnphysics/icepack_atmo.F90

+      !------------------------------------------------------------
+
+      if (sfctype == 'ice') then
+      if (argcheck == 'always' .or. (argcheck == 'first' .and. first_call_ice)) then


The logic in this line is duplicated a few times. Would it make sense to isolate it to a function somewhere ? Then we could use that function instead of the argcheck variable.

The function could receive that boolean fist_call (or here first_call_ice) as argument and return a boolean...

I think that's a good idea, I'll take care of it.

eclare108213

Just a few comments and questions -

I do worry a bit about Phil's comment here, regarding non-Fortran codes being able to use Icepack with Fortran keywords and optional arguments. What should we do about that?

eclare108213 · 2023-01-16T23:46:32Z

columnphysics/icepack_atmo.F90

+         do n = 1, n_iso
+            ratio = c0
+            if (Qa_iso(2) > puny) ratio = Qa_iso(n)/Qa_iso(2)
+            Qref_iso(n) = Qa_iso(n) - ratio*delq*fac


Farther down in the comments for this module, you say

You can definitely get into trouble if you pass the optional argument down the calling tree and use it if it wasn't passed.

So are Qa_iso and Qref_iso safe to use here without checking their 'present' status because the call to this subroutine isn't the first call in which those arguments are declared optional? It appears that you are checking it in what would be the first call into icepack, icepack_atm_boundary. Is there some easy way to track when an optional argument needs to be checked 'if present' and when it doesn't? It's probably not as simple as just checking in icepack's interface routines.

That's right, we have already checked the arguments in the highest level Icepack interface. So they are safe to use here without checking. That's our current design. As I mentioned the other day, there are pros and cons to doing it this way. A pro is that we check all the optional arguments early leveraging parameter flags and then we should be OK after that without having to check over and over. A negative is that you have to keep track of what's going on in different layers of the code.

Okay, thank you.

eclare108213 · 2023-01-16T23:49:42Z

columnphysics/icepack_atmo.F90

+      !------------------------------------------------------------
+      ! Check optional arguments
+      ! This is used for ocean and ice sfctype, need to be careful
+      !------------------------------------------------------------


I think atmo_boundary_layer is also called from the ocean mixed layer routine, in which case the surface type that the atmosphere sees is ocean instead of ice. The mixed layer routine is not used for coupled simulations, usually, and isotopes might not be implemented in a separate ocean component model, which would then require some care here.

eclare108213 · 2023-01-17T00:01:09Z

columnphysics/icepack_snow.F90

@@ -1194,6 +1194,7 @@ subroutine drain_snow (nslyr, vsnon, aicen, &
      else
         sliq = meltsliq  ! computed in thickness_changes
      endif
+!tcx??
      meltsliq = meltsliq


This line can be commented out. This section of the code was rewritten a bit and corrected, and this line was the logical culmination of that process.

Do we want a PR for each feature refactor or just one (or a few) PRs to cover multiple feature refactors? Would like to get some feedback before moving forward.

If I were doing it, I think I would probably do it that way:

* 1 PR per module (so 1 per file) * In each PR, one commit per subroutine: removing `l_*` local variable, using `argcheck` to validate arguments, and adjusting calls to other subroutines if need be.

I don't think 1 PR per module will work well. The optional argument checking will depend on the underlying implementation and that needs to be coordinated. Removing the l_ variables might result in further optimizations down the calling tree. So maybe 1 PR per feature. I also understand about general cleanup (formatting) with updating features. I agree, it's just hard not to fix things when I see an opportunity, and I'm worried I may not circle back if I don't. I'll try to be more careful.

eclare108213 · 2023-01-17T00:08:02Z

columnphysics/icepack_therm_itd.F90

@@ -1852,15 +1853,15 @@ subroutine add_new_ice (ncat,      nilyr,      &
               enddo
            endif

-           frazil_conc = c0


This line is still present in a similar section of code, above. The code is hard-wired for n_iso=3, apparently, so maybe this line should stay in case for some reason n_iso > 3?

eclare108213 · 2023-01-17T00:13:11Z

columnphysics/icepack_therm_vertical.F90

+             (present(fswthru_vdf) .and. .not.present(fswthrun_vdf)) .or. &
+             (present(fswthru_idr) .and. .not.present(fswthrun_idr)) .or. &
+             (present(fswthru_idf) .and. .not.present(fswthrun_idf))) then
+            call icepack_warnings_add(subname//' error in fswthru [iv]d[rf] arguments')


@apcraig @dabail10 Is this the logic we want here? Often it's fine to have category-aggregated variables and not keep the category-specific ones.

It's really the underlying implementation. The implementation in Icepack with this update will always computed the aggregated variables if they are passed, and that requires the category specific arguments too. We could change the implementation so both have to be passed for the category aggregate to be computed. The optional argument checking is determined to a large degree on the underlying implementation.

apcraig · 2023-01-17T23:14:07Z

Just a few comments and questions -

I do worry a bit about Phil's comment here, regarding non-Fortran codes being able to use Icepack with Fortran keywords and optional arguments. What should we do about that?

One option is for the non-Fortran interfaces to NOT adopt the optional arguments in their interfaces/uses. So non-Fortran might pass all arguments even if they are not needed. I don't actually know whether that will work for all languages, but we should explore as an appropriate time. I think for now, we should continue to add optional arguments moving forward. It provide backwards compatibility in the current community. If at some point in the future, we decide to not have optional arguments in icepack, we just remove the optional attribute on the Icepack interfaces and then update any interfaces in CICE/Icepack that do not conform. Everyone in the community would have to do the same, also anytime Icepacks is updated with any new arguments. That's the downside overall. I don't think we should abandon optional arguments until there is a clear reason to do so.

eclare108213 · 2023-01-18T15:54:49Z

Just a few comments and questions -
I do worry a bit about Phil's comment here, regarding non-Fortran codes being able to use Icepack with Fortran keywords and optional arguments. What should we do about that?

One option is for the non-Fortran interfaces to NOT adopt the optional arguments in their interfaces/uses. So non-Fortran might pass all arguments even if they are not needed. I don't actually know whether that will work for all languages, but we should explore as an appropriate time. I think for now, we should continue to add optional arguments moving forward. It provide backwards compatibility in the current community. If at some point in the future, we decide to not have optional arguments in icepack, we just remove the optional attribute on the Icepack interfaces and then update any interfaces in CICE/Icepack that do not conform. Everyone in the community would have to do the same, also anytime Icepacks is updated with any new arguments. That's the downside overall. I don't think we should abandon optional arguments until there is a clear reason to do so.

The voice of reason - sounds good to me.

eclare108213 · 2023-01-19T20:06:57Z

Looks like there are just a few minor changes that could be made here. @apcraig let us know when you're ready to merge.

apcraig · 2023-01-20T10:00:38Z

cheyenne was down this week and i'm away from home, should be able to make progress next few days. will update conversation when updates are ready.

apcraig · 2023-01-23T20:58:51Z

I've updated the PR and running a full test suite now. Added icepack_chkoptargflag logical function and cleaned up a few other things associated with comments above. Feel free to review, will report updated results in the next day or so.

apcraig · 2023-01-24T05:59:25Z

CICE testing is complete with latest changes and everything looks good.

phil-blain

Thanks, latest version looks good!

Refactor isotope implementation with respect to optional arguments

fb45b9e

- Use argcheck design - Eliminate local copies of optional data - Pass optional arguments down the calling tree - Some minor format cleanup

apcraig requested review from eclare108213 and dabail10 January 11, 2023 19:39

apcraig added the Software Engineering label Jan 11, 2023

dabail10 approved these changes Jan 11, 2023

View reviewed changes

eclare108213 reviewed Jan 11, 2023

View reviewed changes

- Continue update of isotope implementation in Icepack including HDO_…

7b2ad22

…ocn, H2_16O_ocn, H2_18O_ocn - Modify fsloss optional argument implementation - Modify rsnwn, smicen, smliqn optional argument implementation

apcraig requested a review from phil-blain January 12, 2023 22:06

- Update fswtrhu_[vi]d[fr] optional argument implementation

0c57334

- Clean up some formatting

apcraig marked this pull request as ready for review January 14, 2023 18:10

apcraig changed the title ~~Refactor isotope implementation with respect to optional arguments~~ Refactor optional argument implementation for isotopes, snwgrain, therm1 and therm2 Jan 16, 2023

phil-blain reviewed Jan 16, 2023

View reviewed changes

eclare108213 reviewed Jan 17, 2023

View reviewed changes

Create icepack_chkoptargflag function for optional argument checking

9e28d92

phil-blain approved these changes Jan 24, 2023

View reviewed changes

eclare108213 approved these changes Jan 24, 2023

View reviewed changes

eclare108213 merged commit 24aef08 into CICE-Consortium:main Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor optional argument implementation for isotopes, snwgrain, therm1 and therm2 #423

Refactor optional argument implementation for isotopes, snwgrain, therm1 and therm2 #423

apcraig commented Jan 11, 2023 •

edited

Loading

apcraig commented Jan 11, 2023

dabail10 left a comment

eclare108213 Jan 11, 2023

apcraig Jan 11, 2023

eclare108213 commented Jan 11, 2023

apcraig commented Jan 14, 2023

apcraig commented Jan 16, 2023

phil-blain Jan 16, 2023

eclare108213 Jan 16, 2023

apcraig Jan 17, 2023

phil-blain commented Jan 16, 2023

phil-blain Jan 16, 2023

apcraig Jan 17, 2023

eclare108213 left a comment •

edited

Loading

eclare108213 Jan 16, 2023

apcraig Jan 17, 2023

eclare108213 Jan 18, 2023

eclare108213 Jan 16, 2023

eclare108213 Jan 17, 2023

apcraig Jan 17, 2023

eclare108213 Jan 17, 2023

eclare108213 Jan 17, 2023

apcraig Jan 17, 2023

apcraig commented Jan 17, 2023

eclare108213 commented Jan 18, 2023

eclare108213 commented Jan 19, 2023

apcraig commented Jan 20, 2023 •

edited

Loading

apcraig commented Jan 23, 2023

apcraig commented Jan 24, 2023

phil-blain left a comment

Refactor optional argument implementation for isotopes, snwgrain, therm1 and therm2 #423

Refactor optional argument implementation for isotopes, snwgrain, therm1 and therm2 #423

Conversation

apcraig commented Jan 11, 2023 • edited Loading

PR checklist

apcraig commented Jan 11, 2023

dabail10 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 commented Jan 11, 2023

apcraig commented Jan 14, 2023

apcraig commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-blain commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclare108213 left a comment • edited Loading

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 Jan 17, 2023

eclare108213 commented Jan 18, 2023

eclare108213 commented Jan 19, 2023

apcraig commented Jan 20, 2023 • edited Loading

apcraig commented Jan 23, 2023

apcraig commented Jan 24, 2023

phil-blain left a comment

Choose a reason for hiding this comment

apcraig commented Jan 11, 2023 •

edited

Loading

eclare108213 left a comment •

edited

Loading

apcraig commented Jan 20, 2023 •

edited

Loading