BLING "spring cleaning" #778

averdy · 2023-10-03T23:59:13Z

What changes does this PR introduce?

Bug fix: add proper initialization of biomass for the 3 phytoplankton groups
New feature: add calculation of chlorophyll concentration as detected by satellite (2D field).

What is the current behaviour?

Unrealistic phytoplankton biomass values at the start of the simulation when using ADVECT_PHYTO flag
Chlorophyll concentration in the top layer is not directly comparable with satellite observations

What is the new behaviour

Realistic phytoplankton biomass values at the start of the simulation
New diagnostic BLGCHLSA is calculated at 13:30 local time based on model chlorophyll concentration in the upper ocean and light attenuation, similarly to how satellites observe ocean color

Does this PR introduce a breaking change?

No

Other information:

Suggested addition to `tag-index`

(To avoid unnecessary merge conflicts, please don't update tag-index. One of the admins will do that when merging your pull request.)

mmazloff · 2023-10-24T17:18:06Z

This looks great to me!

jm-c

I took a quick look, and this looks good. I have 3 comments:

looks like TAF is getting 3 recomputation warnings now, in AD experiment global_oce_biogeo_bling. Now the gradient check did not change at all but seeing some changes to AD-Monitor for adptracer03 and adptracer06 (2 digit match only).
it would be good to have the "local time refinement" to also work when pkg/cal is compiled but not used (useCAL=F) as well as when it's not even compiled, since it's not too difficult to change (see comment below).
the time-window boundaries (12:00 and 13:30) could be changed to parameter, if not run-time params at least fortran PARAMETER (locally in bling_light.F). This could help to follow what the code is doing and also in case this time-window needs to be changed.

jm-c · 2023-11-24T15:01:39Z

pkg/bling/bling_light.F

+C Satellite chlorophyll
+#ifdef ALLOW_CAL
+c mydate is utc time
+        diffutc = XC(i,j,bi,bj)/360.*24


I think it would not be difficult to have something equivalent for the case useCAL = F. I could suggest some changes in this routine if you like.

I'm not sure what the equivalent of the variable localtime is when pkg/cal is not used. Suggestions appreciated!

In addition, the code only makes sense, if XC holds the longitude, i.e. not for cartesian coordinates. Do you want to catch that case?

Ideally yes! And anything else that I've missed, because it's not in the setup I'm used to :)

jm-c · 2023-11-24T15:11:41Z

pkg/bling/bling_bio_nitrogen.F

@@ -1895,6 +1899,8 @@ SUBROUTINE BLING_BIO_NITROGEN(
        CALL DIAGNOSTICS_FILL(N_reminp,  'BLGNREM ',0,Nr,2,bi,bj,myThid)
        CALL DIAGNOSTICS_FILL(P_reminp,  'BLGPREM ',0,Nr,2,bi,bj,myThid)
        CALL DIAGNOSTICS_FILL(no3_adj,   'BLGNONEN',0,Nr,2,bi,bj,myThid)
+C 2d global variables
+        CALL DIAGNOSTICS_FILL(chl_sat,   'BLGCHLSA',0,1,1,bi,bj,myThid)


An alternative would be to move this DIAGNOSTICS_FILL call directly inside bling_light.F where it's computed.
And I did not check carefully, but if variable chl_sat is just used for diagnostics, then it could be changed to a local variable inside bling_light.F and removed from common block. Now this might not be a good idea if you have in mind of computing some cost function from it.

I'm moving the diagnostic_fill call inside bling_light.F as suggested.
chl_sat is going to be used in the cost function, so I'm going to keep it as a global variable.

mjlosch · 2023-11-24T16:21:01Z

pkg/bling/bling_bio_nitrogen.F

+           Phy_lg_local(i,j,k) =  max( epsln, Phy_lg_local(i,j,k) )
+           Phy_sm_local(i,j,k) =  max( epsln, Phy_sm_local(i,j,k) )
+           Phy_diaz_local(i,j,k) =  max( epsln, Phy_diaz_local(i,j,k) )
+


To avoid the new recomputation warnings (the associated recomputations make the run much longer: ALL increases from 128s to 434s, because ADTHE_MAIN_LOOP increases from 52s to 358s) can be avoided without store directives, see below, but most of this routine is actually recomputed in the AD-code and one may want to fix that.

Suggested change

Phy_lg_local(i,j,k) = max( epsln, Phy_lg_local(i,j,k) )

Phy_sm_local(i,j,k) = max( epsln, Phy_sm_local(i,j,k) )

Phy_diaz_local(i,j,k) = max( epsln, Phy_diaz_local(i,j,k) )

#ifdef ALLOW_AUTODIFF_TAMC

C avoid recomputation warnings

ENDIF

ENDDO

ENDDO

ENDDO

DO k=1,Nr

DO j=jmin,jmax

DO i=imin,imax

IF ( maskC(i,j,k,bi,bj).EQ.oneRS ) THEN

#endif

Phy_lg_local(i,j,k) = max( epsln, Phy_lg_local(i,j,k) )

Phy_sm_local(i,j,k) = max( epsln, Phy_sm_local(i,j,k) )

Phy_diaz_local(i,j,k) = max( epsln, Phy_diaz_local(i,j,k) )

mjlosch · 2023-11-27T17:21:23Z

@averdy

I went through bling_bio_nitrogen.F and adjusted store directives and reorganised the code a little to make it more straightforward for TAF. I have now a version that uses fewer store directives (21 instead of 22) and the recomputations seem to be at bay meaning that I think we avoid the expensive part and still do some recomputations where they do not take more than two lines of code, basically just one statement. Shall I push this version of bling_bio_nitrogen.F it, so that you can see if you like it?
I also noticed this code:

C-----------------------------------------------------------
C  avoid negative nutrient concentrations that can result from
C  advection when low concentrations
#ifdef BLING_NO_NEG
      CALL BLING_MIN_VAL(PTR_O2,  1. _d -11, o2_adj, bi, bj)
[...]
#endif

It's not clear to me why this needs to be "hidden from the adjoint" (as stated the routine BLING_MIN_VAL). It would make the adjoint more accurate, if you don't hide it. I think this can be replaced by just capping the input, and avoiding the extra *_adj variables altogether, but I may not understand the meaning of this entirely.

Also there are some fields that are purely diagnostic, e.g. theta_Fe_inv, I suggest to move them to the diagnostics section, so that they are only computed when diagnostics are used (or even when the particular diagnostic is active).
The long "Remineralisation"-loop could be improved/simplified by turning PONflux_u,POPflux_u, PFEflux_u, CaCO3flux_u, SIflux_u into 2D fields, but that's not a priority here.

averdy · 2023-11-29T16:51:45Z

@jm-c Thank you for the feedback - great suggestions! I’m working on it.

averdy · 2023-11-29T16:54:00Z

@mjlosch Thank you for your help! Yes, if you can push your version of bling_bio_nitrogen.F, that would be great. I'm working on addressing your other suggestions.

This change speeds up ADTHE_MAIN_LOOP in verification/global_oce_bio_bling by a factor of 7 (!), mainly because it fixes the extensive recomputations. Relative to the version prior to this PR the speed up is small. With this commit (on my laptop): User/System/Wall clock time: 51.93 sec / 0.30 sec / 52.59 sec Before: User/System/Wall clock time: 358.49 sec / 0.35 sec / 360.67 sec

averdy · 2024-03-26T03:29:59Z

@jm-c Regarding your comment "1" above, I think Martin's edits will have solved the recomputations. Should I do something about "changes to AD-Monitor for adptracer03 and adptracer06"?

In response to comment "3" I'm making the time-window boundaries run-time parameters.

averdy · 2024-03-26T03:48:27Z

@mjlosch Regarding your comment "2": the "adj" variables are meant to keep track for the adjustments for budget calculations. If the bling_min_val code doesn't need to be hidden from the adjoint, should I remove it from bling_ad.flow and bling_ad_diff.list?

I'm moving the purely diagnostic fields to the diagnostics section as suggested in comment "3".

mjlosch · 2024-03-26T07:15:23Z

about comment 2: yes, you could do that, but that will give you recomputation warnings that need to be dealt with via store directives. The question is if you really need that. I can push something, if you like.

Also in bling_ad_diff.list, bling_read_pickup.f (there are flow directives) and bling_write_pickup.f (not called directly) can be removed.

jm-c · 2024-04-04T18:52:41Z

@averdy I saw the 2 new parameters chlsat_tbegin and chlsat_tend, but looks like they are not set in bling_readparms.F (neither default value nor in namelist).

- remove bling_write_pickup.f from bling_ad_diff.list because the call is not visible to taf anyway - remove bling_read_pickup.f from bling_ad_diff.list because there are flow directives (avoid another warning)

… bling_cleanup

jm-c · 2024-04-08T17:20:22Z

@averdy I am going to revert your last commit, because it "undo" all the changes that have been added since last November (and, as a result, this PR now affects 273 files). I went back 1 commit on my clone and I've checked that I have
all the previous changes, including (a) your latest addition to bling_readparms.F, (b) Martin TAF related adjustment, and all the recently merged PR.
Note that If I do this, it will likely make less easy for you to update your working "bling_cleaning" branch and you may want to consider removing this branch from your local clone (but not from your fork !) and checking-out a new local copy of this branch (after updating your clone with a "git fetch").

jm-c · 2024-04-15T03:50:13Z

I remove the last commit by doing:

git reset --hard HEAD~1
git push -f averdy bling_cleanup

so I used a "forced push", and regarding updates, if you already have a local copy of this branch, using simply "git pull" might not work well. For this case, either (a) removing this branch from your local copy ("git branch -D bling_cleanup") and getting it back after a "git fetch", as mentionned earlier, or (b) with "git fetch [remote] ; git checkout bling_cleanup ; git restore . " might do it too.

averdy · 2024-04-15T21:10:45Z

Thank you @jm-c! I've updated my local copy.

mmazloff · 2024-05-10T19:38:47Z

LGTM

jm-c · 2024-05-21T17:07:36Z

@averdy Sorry for waiting so long.
I would like to rename (+ change units) the 2 new parameters chlsat_tbegin and chlsat_tend and make the little changes to allow to use this code without pkg/cal.
I suggest "chlsat_locTimWindow(2)" (a bit long but not used in too many places in the code), in decimal hours,
to be even more clear about local-time (as almost everywhere else time is UTC).
And since these 2 parameters are new (not even there when this PR was submitted) it should not break any set-up (I hope).
If you don't like this please let us know.

averdy · 2024-05-21T17:16:24Z

@jm-c That sounds great!

jm-c · 2024-05-21T19:04:07Z

Also, as I am looking at the details of this local-time window, it seems that the current version is not very accurate:
if it's noon at long=0, I will collect in "chl_sat" the chlorophyll from XC=0 to XC+151.3 (instead of XC+151.5, for a one hour and half longitude band).

jm-c · 2024-05-21T19:05:10Z

so my little changes are going to fix also this inaccuracy.

- rename the 2 new params to "chlsat_locTimWindow(2)" (in decimal hour) - use "decimal hour" consistently (this fix inaccurate time-window criteria). - allow to be used without pkg/cal

just to avoid un-used variables

jm-c

LGTM. I run fwd, adm TAF and Tapenade testreport for exp. global_oce_biogeo_bling and all look good. I see minor changes to AD-monitor for the TAF one but I guess it's expected given the changes in store directives. @mjlosch do you agree ?

jm-c · 2024-05-22T01:58:06Z

@averdy From my side, this PR is good to be merged. So, when you have time, please check/review my set of recent changes and let us know if you aggree or not.

averdy · 2024-05-22T02:25:10Z

@jm-c It looks great!! Thank you so much for improving this code!

mjlosch · 2024-05-22T08:41:03Z

I run fwd, adm TAF and Tapenade testreport for exp. global_oce_biogeo_bling and all look good. I see minor changes to AD-monitor for the TAF one but I guess it's expected given the changes in store directives. @mjlosch do you agree ?

I agree, this looks like the usual round-off issue with recomputation vs restoring from tape (common block). Nothing to worry about.

BLING "spring cleaning"

fd14f6a

jm-c requested a review from mmazloff October 24, 2023 15:37

Merge branch 'master' into bling_cleanup

7ebae16

jm-c self-requested a review November 24, 2023 14:55

jm-c reviewed Nov 24, 2023

View reviewed changes

mjlosch reviewed Nov 24, 2023

View reviewed changes

averdy added 2 commits March 25, 2024 22:32

clean up diagnostics, chl_sat

10d6a12

clean up diagnostics, chl_sat

1f2f230

Merge branch 'master' into bling_cleanup

57aceb2

mjlosch and others added 4 commits April 5, 2024 09:47

adjust cpp-flags to reduce number of taf-not-necessary-warnings

90a633b

make s/r bling_min_val visible to taf, adjust store directives

a4661f4

- remove bling_write_pickup.f from bling_ad_diff.list because the call is not visible to taf anyway - remove bling_read_pickup.f from bling_ad_diff.list because there are flow directives (avoid another warning)

add chlsat_t parameters to namelist and set default values

f3b135f

Merge branch 'bling_cleanup' of https://github.com/averdy/MITgcm into…

5a1d97c

… bling_cleanup

jm-c force-pushed the bling_cleanup branch from 87054e0 to 5a1d97c Compare April 14, 2024 23:28

Merge branch 'master' into bling_cleanup

e2a798f

jm-c added 4 commits May 21, 2024 16:10

remove double blank line

c94ecd4

improve new chl_sat time-window

5fb3bf3

- rename the 2 new params to "chlsat_locTimWindow(2)" (in decimal hour) - use "decimal hour" consistently (this fix inaccurate time-window criteria). - allow to be used without pkg/cal

bring back PR MITgcm#579 changes

2b09c78

just to avoid un-used variables

minor adjustment

e26b4c6

jm-c approved these changes May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLING "spring cleaning" #778

BLING "spring cleaning" #778

averdy commented Oct 3, 2023 •

edited

mmazloff commented Oct 24, 2023

jm-c left a comment

jm-c Nov 24, 2023

averdy Mar 26, 2024

mjlosch Mar 26, 2024

averdy Mar 26, 2024

jm-c Nov 24, 2023

averdy Mar 26, 2024

mjlosch Nov 24, 2023

mjlosch commented Nov 27, 2023

averdy commented Nov 29, 2023

averdy commented Nov 29, 2023

averdy commented Mar 26, 2024 •

edited

averdy commented Mar 26, 2024 •

edited

mjlosch commented Mar 26, 2024

jm-c commented Apr 4, 2024

jm-c commented Apr 8, 2024 •

edited

jm-c commented Apr 15, 2024

averdy commented Apr 15, 2024

mmazloff commented May 10, 2024

jm-c commented May 21, 2024

averdy commented May 21, 2024

jm-c commented May 21, 2024

jm-c commented May 21, 2024

jm-c left a comment •

edited

jm-c commented May 22, 2024

averdy commented May 22, 2024

mjlosch commented May 22, 2024

BLING "spring cleaning" #778

Are you sure you want to change the base?

BLING "spring cleaning" #778

Conversation

averdy commented Oct 3, 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

mmazloff commented Oct 24, 2023

jm-c left a comment

Choose a reason for hiding this comment

jm-c Nov 24, 2023

Choose a reason for hiding this comment

averdy Mar 26, 2024

Choose a reason for hiding this comment

mjlosch Mar 26, 2024

Choose a reason for hiding this comment

averdy Mar 26, 2024

Choose a reason for hiding this comment

jm-c Nov 24, 2023

Choose a reason for hiding this comment

averdy Mar 26, 2024

Choose a reason for hiding this comment

mjlosch Nov 24, 2023

Choose a reason for hiding this comment

mjlosch commented Nov 27, 2023

averdy commented Nov 29, 2023

averdy commented Nov 29, 2023

averdy commented Mar 26, 2024 • edited

averdy commented Mar 26, 2024 • edited

mjlosch commented Mar 26, 2024

jm-c commented Apr 4, 2024

jm-c commented Apr 8, 2024 • edited

jm-c commented Apr 15, 2024

averdy commented Apr 15, 2024

mmazloff commented May 10, 2024

jm-c commented May 21, 2024

averdy commented May 21, 2024

jm-c commented May 21, 2024

jm-c commented May 21, 2024

jm-c left a comment • edited

Choose a reason for hiding this comment

jm-c commented May 22, 2024

averdy commented May 22, 2024

mjlosch commented May 22, 2024

averdy commented Oct 3, 2023 •

edited

Suggested addition to `tag-index`

averdy commented Mar 26, 2024 •

edited

averdy commented Mar 26, 2024 •

edited

jm-c commented Apr 8, 2024 •

edited

jm-c left a comment •

edited