Merge few verification experiments as secondary test in new host exp. #830

jm-c · 2024-04-29T21:25:06Z

What changes does this PR introduce?

Reduce number of compiled experiments (3 fwd, 1 TAF and 1 Tapenade) by merging test set-up into existing experiments:

FWD tests:

flt_example $\rightarrow$ exp4.with_flt
global_with_exf $\rightarrow$ global_ocean_ebm.w_exf
global_with_exf.yearly $\rightarrow$ global_ocean_ebm.yearly
natl_box $\rightarrow$ lab_sea.natl_box
natl_box.longstep $\rightarrow$ lab_sea.longstep

TAF & Tapenade tests:

global_with_exf $\rightarrow$ global_oce_latlon.w_exf

What is the current behaviour?

Similar test experiments that share the same domain (same SIZE.h, ...) and could be run using the same executable
are compiled separately.

What is the new behaviour

Merge few of them as secondary test with minor adjustment to new host executable

Does this PR introduce a breaking change?

No as all changes are in verification/.

Other information:

Just for the purpose of keeping a broad diversity of pkg combinations and pkg/ctrl options in regression tests:

swich off ALLOW_GENTIM2D_CONTROL, ALLOW_DIFFKR_CONTROL and ALLOW_3D_DIFFKR in global_ocean.90x40x15/code_ad: not used, already compiled and used elsewhere + were kept "#undef" in former global_with_exf` TAF AD test. exp.
in global_oce_latlon remove "kpp" (was not used) from both code_ad and code_tap since it's compiled and used elsewhere.
Also use correct CD-Scheme time-stepping in forward experiment global_ocean_ebm as old (and bad) time-stepping is still tested in exp2 FWD test and in this exp. ADM test

Suggested addition to `tag-index`

o verification:

changing domain decomposition (more tiles) in global_with_exf experiment
(both FWD & AD) and update ref. output ;
use correct CD-Scheme time-stepping in FWD exp. global_ocean_ebm and update
ref. output ;
simplify global_ocean.90x40x15/code_ad (undef ALLOW_GENTIM2D_CONTROL,
ALLOW_DIFFKR_CONTROL & ALLOW_3D_DIFFKR); remove kpp from global_oce_latlon
(from both TAF & Tapenade code) ;
after adjusting new host experiment code, merge forward test:
flt_example --> exp4.with_flt
global_with_exf --> global_ocean_ebm.w_exf
global_with_exf.yearly --> global_ocean_ebm.yearly
natl_box --> lab_sea.natl_box
natl_box.longstep --> lab_sea.longstep
and TAF & Tapenade tests:
global_with_exf --> global_oce_latlon.w_exf
For now, keep original set-up in place and add symbolic links into new
host exp. for input dir and ref. output to run these as secondary test.

Old and bad time-stepping still tested in exp2 FWD test and in this exp. ADM test

Changing domain decomposition (more tiles) affects results at machine truncation level (specially fwd gradients).

Turn off un-used code: ALLOW_GENTIM2D_CONTROL, ALLOW_DIFFKR_CONTROL & ALLOW_3D_DIFFKR since already compiled and tested in other set-up (e.g., in global_oce_latlon & global_ocean.cs32x15); and un-comment diffKrT setting (in "data").

Make changes (in code_ad/ and code_tap/) to enable to run "global_with_exf" AD-tests: - compile pkg/exf, remove pkg/kpp (un-used) - define ALLOW_GENTIM2D_CONTROL and adjust CTRL_SIZE.h - import EXF_OPTIONS.h from global_with_exf/code_ad

To enable to run "global_with_exf" FWD-tests: - compile pkgs: bbl, exf, frazil, profiles & diagnostics ; remove ptracers (un-used) - import EXF_OPTIONS.h, PROFILES_OPTIONS.h & DIAGNOSTICS_SIZE.h from global_with_exf/code dir.

To enable to run "flt_example" test: - compile pkg/flt - import FLT_OPTIONS.h from flt_example/code dir.

To enable to run "natl_box" exp. FWD tests

FWD tests: flt_example --> exp4.with_flt global_with_exf --> global_ocean_ebm.w_exf global_with_exf.yearly --> global_ocean_ebm.yearly natl_box --> lab_sea.natl_box natl_box.longstep --> lab_sea.longstep TAF & Tapenade tests: global_with_exf --> global_oce_latlon.w_exf

jm-c · 2024-05-03T18:39:22Z

Few comments:

Before merging this PR, will move the reference output from the 3 previous experiments results/ dir so that these tests are not run twice.
my intention was, for this PR, to leave the input and code dir in the 3 original experiments, with the symbolic links in the new host experiments. And would open a new PR just to clean-up and finish the move. This way the old set-up is still accessible to compare with the new set of tests.
regarding documentation update, could wait for this future/new PR since the old/original set-up will still be available until then (and the new "input" dir is just a link).
there might be other candidate for a merging into new host ? If needed, I could add some comments why I though these 3 were a good choice (from my point of view).

So I think this PR is ready for review.

mjlosch

The changes make a lot of sense. I have a few inline comments, most of them can probably easily be answered or rebutted.

mjlosch · 2024-05-03T13:29:21Z

verification/global_ocean.90x40x15/code_ad/CTRL_OPTIONS.h


 C       >>> Other Control.
 C   Allows for GMREDI controls
 #define ALLOW_KAPGM_CONTROL
 #define ALLOW_KAPREDI_CONTROL
 C   Allows for Vertical Diffusivity controls
-#define ALLOW_DIFFKR_CONTROL
+#undef ALLOW_DIFFKR_CONTROL


Why did we have this defined? Maybe to check the compilation and interaction with other flags?

It's not very clear why ALLOW_DIFFKR_CONTROL is defined in this TAF AD test experiment since (a) it's not used and (b) it is already defined (and sometime used) in other similar, low-res, global or quasi global TAF AD experiments, e.g., in a simpler (linear free-surf) set-up such as global_oce_latlon and a more complex one (with r*) such as global_ocean.cs32x15.
So, just for the purpose of keeping a broad diversity of pkg/ctrl options in regression tests, and since ALLOW_DIFFKR_CONTROL was not defined in global_with_exf/code_ad/, I though it would be a good idea to turn it off in one of the set-up where it's not used.

mjlosch · 2024-05-06T13:22:15Z

verification/global_oce_latlon/code_ad/packages.conf

 gmredi
-kpp
+ggl90


I cannot remember, why kpp was compiled in the experiment. Maybe as an additional test?

This is similar to ALLOW_DIFFKR_CONTROL above but this time with pkg/kpp: It's not used in this TAF AD test but it's compiled but not used in tutorial_tracer_adjsens (there is a comment there in code_ad/packages.conf) and compiled and used in lab_sea experiment.
And since I am already adding a new pkg to compile (i.e., exf) in this test experiment, I though I could remove one un-used one (i.e., kpp) to keep this set-up not complicated.

mjlosch · 2024-05-06T13:25:20Z

verification/global_oce_latlon/code_tap/CTRL_SIZE.h

-      integer     maxCtrlTim2D
-      parameter ( maxCtrlTim2D = 1 )
+      INTEGER     maxCtrlArr3D
+      PARAMETER ( maxCtrlArr3D = 2 )


Would it be a good idea to make the code directories identical between the different ad, tap, oad experiments? Maybe the OAD experiments do not work that way but for tapenade it should, except for the specific flags?

I generally agree with this, specially among all the AD test experiments that share the same set-up (same domain, same resolution, same forcing ...). However, I think this should be addressed in an other PR. And regarding this, this current PR will only make it easier to bring more consistency within AD test experiments (simply by reducing the number).

mjlosch · 2024-05-06T13:28:18Z

verification/global_ocean.90x40x15/input_ad.bottomdrag/data

@@ -8,8 +8,7 @@
 sRef = 15*35.,
 viscAr=1.E-3,
 diffKhT=0.,
-#- diffKrT unused when compiled with ALLOW_3D_DIFFKR
-#diffKrT=3.E-5,
+ diffKrT=3.E-5,


doesn't this change the experiment now with ALLOW_3D_DIFFKR undefined?

@mjlosch I am not sure what you mean here: As I turned off ALLOW_3D_DIFFKR, I had to activate (i.e., remove the "#" in front of) diffKrT setting in main parameter file "data", and the results are unchanged.
So, from my point of view, I don't think this experiment really changes (same results as before).

mjlosch · 2024-05-06T13:32:13Z

verification/global_with_exf/code/SIZE.h

@@ -42,12 +42,12 @@ CEOP
      INTEGER Ny
      INTEGER Nr
      PARAMETER (
-     &           sNx =  45,
-     &           sNy =  40,
+     &           sNx =  30,


change the experiment?

mjlosch · 2024-05-06T13:33:44Z

verification/global_with_exf/code_ad/SIZE.h

@@ -43,11 +43,11 @@ CEOP
      INTEGER Nr
      PARAMETER (
     &           sNx =  45,
-     &           sNy =  40,
+     &           sNy =  20,


not clear: the tests are modified, but at the same time it shall be merged into a different one (global_ocean_ebm)?

I think this is the same asw above, except the the new host experiment is different (so are the tile size).

jm-c · 2024-05-07T19:53:18Z

@mjlosch Thanks for the review. Will try to answer your points/questions later.

jm-c · 2024-05-13T04:59:10Z

@mjlosch I tried to answer your points, and let me know if this is still not clear. Also, since I added many sim-link, the github display of the set of changes (from PR web-page) might not be easy to interpret (but would be more clear from a checkout of this branch).

mjlosch

@jm-c addressed my previous comments (which were only comments anyway).

I think that this looks good and will reduce number of compilations.

in experiments that are merged

was used before adding pkg/monitor to code_tap/packages.conf

jm-c · 2024-05-14T15:46:10Z

@mjlosch Thanks for the review. I updated the PR description ("Other information" section) to mention side updates (some related to your previous questions). Will fill the suggested addition to tag-index and tweak the reference output to skip the old tests.

To avoid compiling and running the merged experiments twice, rename reference output in former exp. results dir (just keep secondary test in new host experiment).

jm-c · 2024-05-14T18:02:11Z

I think this PR is now ready to be merged. @mjlosch do you agree ?
If yes and if there is not other comments, I will merge it in the coming few days.

include a few blank lines and "` `"

mjlosch · 2024-05-15T05:49:35Z

@jm-c I think this is ready to be merged. I took the liberty and fixed (and broke and fixed) some of the modified markdown README.md files. (It is really annoying that the Markdown renderer of GitHub seems to be very different from my offline version, see lab_sea/README.md changes). If you don't like them we can easily revert them.

jm-c · 2024-05-15T15:01:57Z

@mjlosch I am going to revert changes in 2 previously unchanged README (in exp4 and in lab_sea) since

these are in new host experiments and will be updated in the coming PR that move the input dir, as mentionned earlier:

regarding documentation update, could wait for this future/new PR since the old/original set-up will still be available
until then (and the new "input" dir is just a link).

the changes you made did not really document the new secondary test (so no overlap with point (1) above).
in the case of lab_sea, since the README there was updated recently in PR lab_sea verification descriptions #807, which only did that, it's probably better to keep it unchanged for now.

And regarding the 3 other, I am going to bring back some double-quote that I put intentionally (and consistently) and adjust/remove some blank lines (as in tutorial_baroclinic_gyre/README.md).

jm-c · 2024-05-15T15:34:49Z

@mjlosch a side comment about this:

It is really annoying that the Markdown renderer of GitHub seems to be very different from my offline version

To check how Markdown render on GitHub web-page, I usually go the web-page and try things with the "edit" buttom and "preview". And when I am happy with it, I cancel the changes (from the web-browser) and put them back into my local ".md" file to make a proper commit.

jm-c · 2024-05-15T16:04:02Z

Going to merge this PR soon.

jm-c added 13 commits April 28, 2024 00:03

Change domain decomposition to use 6 tiles

880118d

Change domain decomposition to use 4 tiles

859339d

Simpler (could skip input/prepare_run)

694976a

smaller Overlap-size (reduce from 3 to 2)

7f7b150

Use correct CD-Scheme time-stepping

40a4b74

Old and bad time-stepping still tested in exp2 FWD test and in this exp. ADM test

Update FWD output (fix CD-scheme time-stepping)

e93904f

Update ref. output

efb419b

Changing domain decomposition (more tiles) affects results at machine truncation level (specially fwd gradients).

Simplify TAF AD-code in global_ocean.90x40x15 exp.

f76afc8

Turn off un-used code: ALLOW_GENTIM2D_CONTROL, ALLOW_DIFFKR_CONTROL & ALLOW_3D_DIFFKR since already compiled and tested in other set-up (e.g., in global_oce_latlon & global_ocean.cs32x15); and un-comment diffKrT setting (in "data").

Update global_ocean_ebm/code/:

16c553c

To enable to run "global_with_exf" FWD-tests: - compile pkgs: bbl, exf, frazil, profiles & diagnostics ; remove ptracers (un-used) - import EXF_OPTIONS.h, PROFILES_OPTIONS.h & DIAGNOSTICS_SIZE.h from global_with_exf/code dir.

Update exp4/code/:

1f542ba

To enable to run "flt_example" test: - compile pkg/flt - import FLT_OPTIONS.h from flt_example/code dir.

Add pkgs ptracers & longstep

62a6205

To enable to run "natl_box" exp. FWD tests

jm-c added work in progress Should not be merged until this label is removed adjoint Affects the adjoint model; label triggers full OpenAD test labels Apr 29, 2024

jm-c removed the work in progress Should not be merged until this label is removed label May 3, 2024

jm-c requested a review from mjlosch May 6, 2024 19:24

mjlosch reviewed May 6, 2024

View reviewed changes

mjlosch self-requested a review May 13, 2024 11:57

mjlosch approved these changes May 13, 2024

View reviewed changes

jm-c added 3 commits May 13, 2024 09:44

Update README.md

3d4ba71

in experiments that are merged

Really update README.md

fb4c914

in experiments that are merged

remove un-used "data.no_mon"

3cc2720

was used before adding pkg/monitor to code_tap/packages.conf

rename reference output file

17dcd56

To avoid compiling and running the merged experiments twice, rename reference output in former exp. results dir (just keep secondary test in new host experiment).

small modifs to markdown files to make the more robust (?)

c8714b2

include a few blank lines and "` `"

mjlosch added 2 commits May 15, 2024 07:42

turn README into markdown file README.md

bc5f033

fix lab_sea/README.md (almost back to previous version)

798e001

keep previously unchanged README

fa74659

small adjustments

0a44248

jm-c added 2 commits May 15, 2024 11:39

Merge branch 'master' into fewer_exp_to_compile

4c7270f

document merging few experiments

457d82c

jm-c merged commit 0c90ef5 into MITgcm:master May 15, 2024
23 checks passed

jm-c mentioned this pull request May 16, 2024

Fix for Multi-threaded test #834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge few verification experiments as secondary test in new host exp. #830

Merge few verification experiments as secondary test in new host exp. #830

jm-c commented Apr 29, 2024 •

edited

jm-c commented May 3, 2024

mjlosch left a comment

mjlosch May 3, 2024

jm-c May 13, 2024

mjlosch May 6, 2024

jm-c May 13, 2024

mjlosch May 6, 2024

jm-c May 13, 2024

mjlosch May 6, 2024

jm-c May 13, 2024

mjlosch May 6, 2024

mjlosch May 6, 2024

jm-c May 13, 2024

jm-c commented May 7, 2024

jm-c commented May 13, 2024

mjlosch left a comment

jm-c commented May 14, 2024

jm-c commented May 14, 2024

mjlosch commented May 15, 2024

jm-c commented May 15, 2024

jm-c commented May 15, 2024

jm-c commented May 15, 2024

Merge few verification experiments as secondary test in new host exp. #830

Merge few verification experiments as secondary test in new host exp. #830

Conversation

jm-c commented Apr 29, 2024 • 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 commented May 3, 2024

mjlosch 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

jm-c commented May 7, 2024

jm-c commented May 13, 2024

mjlosch left a comment

Choose a reason for hiding this comment

jm-c commented May 14, 2024

jm-c commented May 14, 2024

mjlosch commented May 15, 2024

jm-c commented May 15, 2024

jm-c commented May 15, 2024

jm-c commented May 15, 2024

jm-c commented Apr 29, 2024 •

edited

Suggested addition to `tag-index`