OBCS_SEAICE_NEUMANN flag, more store directives, and cleanup #740

mjlosch · 2023-06-26T11:30:17Z

What changes does this PR introduce?

new feature and some additional tweaks of TAF store directives and cleanup

What is the current behaviour?

no Neumann boundary condition for sea ice variables
defining ALLOW_OBCS_BALANCE and ALLOW_OBCS_STEVENS leads to (hidden) recomputations and also to some recomputation warnings (untested configuration)

What is the new behaviour

Neumann boundary conditions for sea ice variables, activate by defining new flag OBCS_SEAICE_NEUMANN, defining this flag will override any other input from files for sea ice variables.
fewer (hidden) recomputations and no recomputation warnings when both ALLOW_OBCS_BALANCE and ALLOW_OBCS_STEVENS and OBCS_SEAICE_NEUMANN are defined
new runtime flag for pkg/cost: cost_mask_file, which can be useful for flexible cost function definitions

Does this PR introduce a breaking change?

No

Other information:

These are changes that were required for a specific configuration. I think they are useful for everyone.

Suggested addition to `tag-index`

o pkg/obcs:

Neumann boundary conditions for sea ice variables, activate by defining new flag OBCS_SEAICE_NEUMANN
fewer (hidden) recomputations and no recomputation warnings when both ALLOW_OBCS_BALANCE and ALLOW_OBCS_STEVENS and OBCS_SEAICE_NEUMANN are defined
o pkg/cost: new runtime flag for pkg/cost: cost_mask_file, which can be useful for flexible cost function definitions

while not used in the current code, it can be useful to have simple mask for flexible cost function definition

- when defined, boundary value are computed from interior fields so that the normal derivative across the boundary is zero (for pkg/seaice variables only)

- also make sure that do_oceanic_phys, seaice_model, obcs_calc is not recomputed in parts or totally (without TAF-warnings)

to resolve conflicts

jm-c · 2024-04-10T04:08:17Z

pkg/obcs/obcs_apply_uvice.F

@@ -77,7 +75,12 @@ SUBROUTINE OBCS_APPLY_UVICE(
         DO i=1-OLx,sNx+OLx
          Jobc = OB_Jn(i,bi,bj)
          IF ( Jobc.NE.OB_indexNone ) THEN
+#  ifdef SEAICE_OBCS_NEUMANN
+C     Neumann boundary conditions
+           uFld(i,Jobc,bi,bj) = uFld(j,Jobc-1,bi,bj)


should this be "= uFld(i,Jobc-1,bi,bj)" instead ?

jm-c

I took a look at code changes and have few comments:

There are many TAF store dir changes, and few changes to testreport AD output (all 4 global_ocean.90x40x15 tests + global_ocean_ebm + tutorial_global_oce_optim), all small differences and some in forward grad-check and some in AD grad-check + AD monitor. Will need to run this on villon to check if I need to update few ref-output. Also need to run with Tapenade since I notices few changes related to Tapenade code.
there is a new CPP "OBCS_SEAICE_NEUMANN" in SEAICE_OPTIONS.h but I see few "#ifdef SEAICE_OBCS_NEUMANN" in obcs_apply_uvice.F & obcs_apply_seaice.F.
Also,since it does not involve new arrays in common block, would it be difficult to have a run-time switch instead of a CPP option ?
At some point, it would be good to get a minimal doc of pkg/obcs implemetation in the manual (currently 8.3.1.6 and 8.3.1.5.10 are empty), specially for seaice since it does not follow the same sequence (e.g., obcs_apply_seaice called at the very end, after all pkg/seaice update but obcs_apply_uvice called before any update of pkg/seaice fields ).
Not sure it would be better, but could we skip the changes in obcs_apply_uvice.F and set OB[N,S,E,W]u,vice instead ?
very minor, for "cost config" report in STDOUT, I like what pkg/ctrl reports:

control vector configuration >>> START <<<
control vector configuration >>> END <<<

and could use the same convention here.

jm-c · 2024-04-12T14:20:13Z

Detail: I don't see anymore any change related to "divisions by zero in taf-generated ad-code of ggl90_calc.F", so may be the PR description could be updated.

mjlosch · 2024-04-15T06:27:40Z

@jm-c thanks for having a look and for you comments

There are many TAF store dir changes, and few changes to testreport AD output (all 4 global_ocean.90x40x15 tests + global_ocean_ebm + tutorial_global_oce_optim), all small differences and some in forward grad-check and some in AD grad-check + AD monitor. Will need to run this on villon to check if I need to update few ref-output. Also need to run with Tapenade since I notices few changes related to Tapenade code.

This is not the cleanest PR, because it mixes the Neumann BCs for seaice with other stuff. The "other" stuff is more reorganisation of store directives to avoid extra computations where they are not necessary (e.g. 85ec739. This reorders the AD code and I can imagine that there are small changes due to reading tapes instead of recomputing (truncation). Not sure where the FW differences come from. I will have another look to make sure that everything is OK with that.
update: On my MacBook with gfortran, where results are not perfect w.r.t. the reference to begin with, I have only these differences (for the three experiments listed above):

< Y Y Y Y 14>12< 7 13 12 13 13 14 16 16 16 16 16 14 14 14 16 16 16 pass  global_ocean.90x40x15.bottomdrag (this branch)
---
> Y Y Y Y 14>12< 7 13 12 13 13 14 16 13 16 16 16 14 14 14 13 16 16 pass  global_ocean.90x40x15.bottomdrag (master)

i.e. the agreement with the reference even improves a little. This somewhat supports my feeling that the differences is a truncation error issue when replacing recomputation by tapes.

there is a new CPP "OBCS_SEAICE_NEUMANN" in SEAICE_OPTIONS.h but I see few "#ifdef SEAICE_OBCS_NEUMANN" in obcs_apply_uvice.F & obcs_apply_seaice.F.

That's embarrassing and will be fixed. It's a bad idea not to test new code ... will think of a verification experiment where I can add this functionality.

Also,since it does not involve new arrays in common block, would it be difficult to have a run-time switch instead of a CPP option ?

That should be possible. I didn't do it because I thought that extra "if" would make the code less efficient. Do think that this type of code would be OK? Would the runtime flag SEAICEuseNeumannBC be part of data.obcs or data.seaice

          IF ( Jobc.NE.OB_indexNone ) THEN
           IF ( SEAICEuseNeumannBC ) THEN
C     Neumann boundary conditions
            HEFF (i,Jobc,bi,bj) = HEFF (i,Jobc-1,bi,bj)
            AREA (i,Jobc,bi,bj) = AREA (i,Jobc-1,bi,bj)
            HSNOW(i,Jobc,bi,bj) = HSNOW(i,Jobc-1,bi,bj)
#  ifdef SEAICE_VARIABLE_SALINITY
            HSALT(i,Jobc,bi,bj) = HSALT(i,Jobc-1,bi,bj)
#  endif
           ELSE
            HEFF (i,Jobc,bi,bj) = OBNh (i,bi,bj)
            AREA (i,Jobc,bi,bj) = OBNa (i,bi,bj)
            HSNOW(i,Jobc,bi,bj) = OBNsn(i,bi,bj)
#  ifdef SEAICE_VARIABLE_SALINITY
            HSALT(i,Jobc,bi,bj) = OBNsl(i,bi,bj)
#  endif
           ENDIF
          ENDIF

At some point, it would be good to get a minimal doc of pkg/obcs implemetation in the manual (currently 8.3.1.6 and 8.3.1.5.10 are empty), specially for seaice since it does not follow the same sequence (e.g., obcs_apply_seaice called at the very end, after all pkg/seaice update but obcs_apply_uvice called before any update of pkg/seaice fields ).

That would be great. There are so many sea ice options that I did not implement, so that I am not sure that could describe them properly, but we could start doing so in this PR. I'll give it a shot.

Not sure it would be better, but could we skip the changes in obcs_apply_uvice.F and set OB[N,S,E,W]u,vice instead ?

Would be more efficient, see 3. point

very minor, for "cost config" report in STDOUT, I like what pkg/ctrl reports:

control vector configuration >>> START <<<
control vector configuration >>> END <<<

and could use the same convention here.

Yes, we could modify cost_readparms.F in this direction, will try.

as a basis for further improvement

jm-c · 2024-04-16T03:42:26Z

@mjlosch I have not looked at your latest changes but few comments regarding your answers:

since I wrote my last set of comments, I did run a Tapenade testreport (AD) and everything is good.
the run-time switch vs CPP option: Since there is already a "IF ( I/Jobc.NE.OB_indexNone ) THEN" inside the loop, with I/Jobc dependency on loop index, it might not make a big difference to have an other "IF / THEN / ELSE" inside the same loop. So the run-time option might be a good thing.
regaring point 5:

Not sure it would be better, but could we skip the changes in obcs_apply_uvice.F and set OB[N,S,E,W]u,vice instead ?

this could be left for later, because I am not sure it would be an improvement and also how this will work with future seaice + obcs development ? not clear to me.

I will take a look at the new docs (exiting !).

first attempt

mjlosch · 2024-04-16T08:02:34Z

in d8ac131 I applied some sort of hybrid: Along with replacing the CPP flag with a runtime flag for Neumann BCs, I copied the corresponding uIce/vIce to the OB?u/vice within the routine obcs_apply_uvice.F. It's not ideal, but this block could be moved easily, but where? It would be logical to put it into obcs_calc.F, but after call obcs_prescribe_read, right?

jm-c · 2024-04-16T12:45:03Z

@mjlosch regarding this:

It would be logical to put it into obcs_calc.F, but after call obcs_prescribe_read, right?

Should this be after the OBCS_CONTROL ? or at the same level as OBCS_STEVENS ? (and in this case just after ?).

mjlosch · 2024-04-16T13:36:35Z

neither ctrl_getobcs nor obcs_calc_stevens does anything to sea ice, so we are free to put in anywhere.

I guess putting this at the end after obcs_calc_stevens makes the most sense to me, if we agree that it is supposed to override any other boundary condition. I should create a regression test for this anyway before I start moving stuff around.

addressed unwarned recomputations and some new recomputation warnings associated with obcs

plus useSeaiceNeumann boundary conditions, aEVP, Visbeck97 in AD mode

mjlosch · 2024-04-19T14:56:17Z

I now added a forward test of the sea ice Neumann boundary conditions to seaice_obcs.tides, which let me immediately catch two errors in the process, commit aa27cd6

Everything after commit 5c100af is preparation for a TAF-AD test for seaice+obcs+Neumann (+and aEVP). This new test required a few adjustments. Especially 031eb42 may appear as an overkill, but there were new RECOMPUTATION warnings in EVP and I used this to fix the EVP AD code. In the test it works nicely.

I had to turn off tides in AD because they lead to massive recomputation warnings which I couldn't fix quickly. That's something on the to-do list.

Both reference solutions are generated on my MacBook and will require updates from the reference machine.

... and the torture never stops (F.Z.)

mostly to test the Neumann boundary code for seaice

jm-c · 2024-05-23T13:59:49Z

@mjlosch I reverted to changes in seaice_obcs/input.tides/ to be able to merge master branch in (this test exp got changed a lot with PR #752). I will now merge master in and let you decide if you want to bring back your changes (I kept the modified files somewhere).

mjlosch · 2024-05-23T14:09:24Z

The reverted changes test the seaice Neumann code in a forward experiment (the new test in offline_exf_seaice is an AD test, but certainly the forward part is tested, too). I know that the seaice_obcs experiment is not thought to be so great, but we can do this here without additional compilation, so I thought that this is convenient. I opt for putting it back.

jm-c · 2024-05-23T15:09:03Z

@mjlosch OK, I will put it back (easy and far less changes than from PR #752).

and update reference output (from ref. machine "villon")

jm-c · 2024-05-23T23:11:58Z

model/src/initialise_varia.F

@@ -107,9 +107,13 @@ SUBROUTINE INITIALISE_VARIA( myThid )
 #include "PARAMS.h"
 #include "DYNVARS.h"
 #include "SURFACE.h"
-#ifdef ALLOW_AUTODIFF
+#ifdef ALLOW_AUTODIFF_TAMC


I suggest to keep the ALLOW_AUTODIFF instead. Since it's not obvious that the following included headers will not be used it using an other AD-tool, and it's not very specific to TAF (like storage directive), we can keep it general.

jm-c · 2024-05-23T23:37:22Z

pkg/obcs/obcs_fields_load.F

@@ -72,7 +72,8 @@ SUBROUTINE OBCS_FIELDS_LOAD( myTime, myIter, myThid )

 C     if external forcing (exf) package is enabled (useEXF=T), all loading of
 C     external fields is done by exf
-#if (defined ALLOW_OBCS && defined ALLOW_OBCS_PRESCRIBE )
+#if ( defined ALLOW_OBCS && defined ALLOW_OBCS_PRESCRIBE && \
+      !defined EXCLUDE_FFIELDS_LOAD )


I would prefer to split this into 2 "#ifdef" : this continuation line is making the search for active CPP options so much more tricky (+ it tricks also my editor).

jm-c · 2024-05-23T23:43:27Z

pkg/obcs/obcs_init_variables.F

@@ -361,6 +364,26 @@ SUBROUTINE OBCS_INIT_VARIABLES( myThid )
      IF ( nIter0.EQ.0 .AND. pickupSuff.EQ.' ' ) THEN
 #ifdef ALLOW_DEBUG
       IF (debugMode) CALL DEBUG_CALL('OBCS_CALC',myThid)
+#endif
+#ifdef ALLOW_AUTODIFF_TAMC
+# ifdef ALLOW_OBCS_STEVENS


but here i would not mind having the ALLOW_AUTODIFF_TAMC and ALLOW_OBCS_STEVENS combined (since @mjlosch you made similar changes in this PR).

jm-c · 2024-05-24T15:19:37Z

@mjlosch I looked at the recent changes and this looks good (had few minor comments related to CPP conditions I mentioned yesterday). You can also check the comments I updated (in obcs_apply_uvice.F).

And before merging this PR I will run again testreport on ref. machine to check if/which ref. output needs an update.

mjlosch · 2024-05-24T16:06:25Z

New comments look good to me. I followed your CPP-flag ideas. I am fine with this now. The new verification experiment takes a while (96.87sec vs 27.78 of the default one).

jm-c · 2024-05-24T22:12:00Z

I ended up adding file output_tlm.obcs.txt.gz in offline_exf_seaice/results so that the tang-lin will also be tested.
And updated more output that strickly necessary to also get better FWD grad agreement.

mjlosch added 5 commits June 26, 2023 10:10

avoid division by zero in AD code of SQRT

99042a8

cleanup and reordering of CPP flags

32b24d5

add cost_mask_file to be specified at runtime

f3b8229

while not used in the current code, it can be useful to have simple mask for flexible cost function definition

add new flag OBCS_SEAICE_NEUMANN which overrides input files

d906bf6

- when defined, boundary value are computed from interior fields so that the normal derivative across the boundary is zero (for pkg/seaice variables only)

adjust store directives to work with stevens BC and BALANCE_FLOW

85ec739

- also make sure that do_oceanic_phys, seaice_model, obcs_calc is not recomputed in parts or totally (without TAF-warnings)

mjlosch added the adjoint Affects the adjoint model; label triggers full OpenAD test label Jun 26, 2023

mjlosch requested review from jm-c and antnguyen13 June 26, 2023 11:30

mjlosch and others added 6 commits July 10, 2023 12:34

avoid multiple divisions by constants, some cleanup

ad38f22

Merge branch 'upstream/master'into sochic_code_changes

886addf

to resolve conflicts

fix making the wrong decision during the merge

8dc7a3d

Merge upstream/master into sochic_code_changes

6694d02

Merge branch 'master' into sochic_code_changes

fcb6ccd

rename "ctrl_dummy.h" --> "CTRL_DUMMY.h"

96f4ddd

jm-c reviewed Apr 10, 2024

View reviewed changes

mjlosch added 2 commits April 10, 2024 08:46

fix indexing bug spotted by jmc

f926ef2

fix another bug introduced in commit d906bf6

c79648d

jm-c reviewed Apr 12, 2024

View reviewed changes

mjlosch added 3 commits April 15, 2024 09:50

small format changes to configuration output

2a5b546

add minimal documentation to seaice with obcs and a flow chart

d79103a

as a basis for further improvement

small change to doc

668a88d

turn Neumann BC CPP-flag into runtime flag useSeaiceNeumann

d8ac131

first attempt

remove unnecessary store directives

6ab305f

mjlosch mentioned this pull request Apr 17, 2024

Include pkg/steep_icecavity by leveraging pkg/shelfice files #789

Merged

mjlosch added 7 commits April 18, 2024 10:12

test useSeaiceNeumann in seaice_obcs.tides

5c100af

small bug fix

2ddc06b

include OBCS_PARAMS.h to be able to compile with pkg/seaice and obcs

4564843

new store directives necessary for undef EXCLUDE_PCELL_MIX_CODE

33cf716

adjust store directives to avoid recomputations, fix division by 0

031eb42

addressed unwarned recomputations and some new recomputation warnings associated with obcs

only check LSR constants in AD-mode if SEAICEuseLSR=T

5a4ce7b

add TAF-AD verification experiment that test obcs + seaice

16d4706

plus useSeaiceNeumann boundary conditions, aEVP, Visbeck97 in AD mode

mjlosch and others added 6 commits April 22, 2024 11:19

simplify test: do not deal with Visbeck AD now

aa8c23c

small adjustments in store directives with a big effect

fe4a92e

... and the torture never stops (F.Z.)

AD verification experiment now with tides code (only compiled)

37f160e

new secondary test with 4 open boundaries

9c0b37f

mostly to test the Neumann boundary code for seaice

remove AD-configuration of seaice_obcs

5c3878b

back to original version

33f3fc7

Merge branch 'master' into sochic_code_changes

7f73d3e

bring back useSeaiceNeumann test

05b89b8

and update reference output (from ref. machine "villon")

jm-c reviewed May 23, 2024

View reviewed changes

jm-c added 2 commits May 23, 2024 19:51

update comments

0794cb5

update comments

848d369

adjust some CPP-flags

6603bd3

jm-c added 2 commits May 24, 2024 15:31

more consistent upper/lower case

edb53a4

Update some adm & tlm ref. output

bd45a19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBCS_SEAICE_NEUMANN flag, more store directives, and cleanup #740

OBCS_SEAICE_NEUMANN flag, more store directives, and cleanup #740

mjlosch commented Jun 26, 2023 •

edited

jm-c Apr 10, 2024

jm-c left a comment •

edited

jm-c commented Apr 12, 2024

mjlosch commented Apr 15, 2024 •

edited

jm-c commented Apr 16, 2024

mjlosch commented Apr 16, 2024

jm-c commented Apr 16, 2024

mjlosch commented Apr 16, 2024

mjlosch commented Apr 19, 2024

jm-c commented May 23, 2024

mjlosch commented May 23, 2024

jm-c commented May 23, 2024

jm-c May 23, 2024

jm-c May 23, 2024

jm-c May 23, 2024

jm-c commented May 24, 2024

mjlosch commented May 24, 2024

jm-c commented May 24, 2024

OBCS_SEAICE_NEUMANN flag, more store directives, and cleanup #740

Are you sure you want to change the base?

OBCS_SEAICE_NEUMANN flag, more store directives, and cleanup #740

Conversation

mjlosch commented Jun 26, 2023 • edited

What changes does this PR introduce?

What is the current behaviour?

What is the new behaviour

Does this PR introduce a breaking change?

Other information:

Suggested addition to tag-index

jm-c Apr 10, 2024

Choose a reason for hiding this comment

jm-c left a comment • edited

Choose a reason for hiding this comment

jm-c commented Apr 12, 2024

mjlosch commented Apr 15, 2024 • edited

jm-c commented Apr 16, 2024

mjlosch commented Apr 16, 2024

jm-c commented Apr 16, 2024

mjlosch commented Apr 16, 2024

mjlosch commented Apr 19, 2024

jm-c commented May 23, 2024

mjlosch commented May 23, 2024

jm-c commented May 23, 2024

jm-c May 23, 2024

Choose a reason for hiding this comment

jm-c May 23, 2024

Choose a reason for hiding this comment

jm-c May 23, 2024

Choose a reason for hiding this comment

jm-c commented May 24, 2024

mjlosch commented May 24, 2024

jm-c commented May 24, 2024

mjlosch commented Jun 26, 2023 •

edited

Suggested addition to `tag-index`

jm-c left a comment •

edited

mjlosch commented Apr 15, 2024 •

edited