3D pH pickup for pkg/DIC calcite saturation #718

seamanticscience · 2023-04-05T00:20:45Z

What changes does this PR introduce?

Read and write a pickup for 3D pH field for use when #define DIC_CALCITE_SAT

What is the current behaviour?

3D pH is initialized from a first guess cold start each time, requiring iterations of the carbonate solver over the full model domain.

What is the new behaviour

A new pickup file

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)

Other information:

Moved some 3D silicate initialization from S/R DIC_SURFFORCING_INIT to S/R DIC_INI_FORCING.

Suggested addition to `tag-index`

o 3D pH pickup for calcite saturation calculation

mjlosch

I have these comments after reading the code and the changes. I cannot claim that I fully understand, what's going on here, so any of my comments may be entirely useless (in which case I apologise in advance).

The (fundamental) difference between dic_ini_forcing.F and dic_surfforcing_init.F is not clear to me. Why are there two different files/routines?
this makes it difficult to understand why parts of the code are now moved from one file to the other, and why some variables are initialised multiple times.
silicaSurf is a surface forcing field, silicaDeep is an initial 3D field (but not a forcing field). Still, both are initialised in dic_ini_forcing.F, which is a bit misleading.
I would recommend initialise all fields once in dic_init_varia.F, then read initial fields and/or the pickup file(s) (maybe also from dic_init_varia.F), and then do something with the initial fields if necessary in dic_ini_forcing.F, possibly combining it with dic_surfforcing_init.F
maybe it is possible to reuse calcite_saturation.F instead of duplicating (most of) the code

mjlosch · 2023-06-22T13:59:14Z

pkg/dic/dic_ini_forcing.F

+CC Not sure if these all need reinitializing here?
+       CALL LEF_ZERO(ak0, myThid)
+       CALL LEF_ZERO(ak1, myThid)
+       CALL LEF_ZERO(ak2, myThid)
+       CALL LEF_ZERO(akw, myThid)
+       CALL LEF_ZERO(akb, myThid)
+       CALL LEF_ZERO(akf, myThid)
+       CALL LEF_ZERO(ak1p, myThid)
+       CALL LEF_ZERO(ak2p, myThid)
+       CALL LEF_ZERO(ak3p, myThid)
+       CALL LEF_ZERO(aksi, myThid)
+       CALL LEF_ZERO(fugf, myThid)
+       CALL LEF_ZERO(ff, myThid)
+       CALL LEF_ZERO(ft, myThid)
+       CALL LEF_ZERO(st, myThid)
+       CALL LEF_ZERO(bt, myThid) 


I am not sure what's happening here exactly, but typically we would initialise all global variables that (have the potential to) vary during the run in dic_init_varia.F. Now that file is a little underpopulated. Instead, these variables are initialised here to zero and again to zero in dic_surfforcing_init.F (along with some other variables), after they have been used (with zeros?).

Ok, I can do this, but it that doesn't completely clear everything up - also looks like fixed values (alpha and rain_ratio) are initialized in S/R dic_ini_varia, instead of S/R dic_ini_fixed. I think this would be better as a separate P/R to streamline pkg/dic initialization.

mjlosch · 2023-06-22T14:00:15Z

pkg/dic/dic_calcite_read_pickup.F

+      INTEGER length_of_rec
+      _RL vec(1-Olx:sNx+Olx,1-Oly:sNy+Oly,Nr,nSx,nSy)
+CEOP
+#ifdef DIC_CALCITE_SAT


wouldn't it be better to add this to dic_read_pickup.F instead of creating a new file? The variable pH3DisLoaded could a common block variable.

@jm-c needs to confirm that, but I thing that we can append pickup files (instead of creating new ones).

mjlosch · 2023-06-22T14:20:16Z

pkg/dic/dic_ini_forcing.F

 #endif
-C-    end bi,bj loops
+C Calculate the inorganic carbon sytem disociation coefficients at the surface


From now on it's more or less a replica of calcite_saturation.F except of the use of AHINI_FOR_AT which is commented out in calcite_saturation.F
can't we just call calcite_saturation from here and do something with AHINI_FOR_AT (call only if myTime .eq. startTime or similar)?

seamanticscience · 2023-06-30T15:23:04Z

Thanks for your comments @mjlosch

The (fundamental) difference between dic_ini_forcing.F and dic_surfforcing_init.F is not clear to me. Why are there two different files/routines?

Good question, I'm just working within the framework of the current code so I don't have a good answer. They do both deal with different forcing at the surface, but S/R dic_surfforcing_init mostly relates to CO2 fluxes (initializing the "atmospheric carbon box", the dissociation coefficients, and the surface pH/pCO2.

this makes it difficult to understand why parts of the code are now moved from one file to the other, and why some variables are initialised multiple times.

Right, that is partly me being over-cautious. The ak arrays are 2d, and are reused for the surface pH/pCO2 calculations, and for the 3d pH/carbonate saturation calculations, looping over k. They probably don't need to be zeroed out each time.

silicaSurf is a surface forcing field, silicaDeep is an initial 3D field (but not a forcing field). Still, both are initialised in dic_ini_forcing.F, which is a bit misleading.

silicaDeep is also a forcing prescribed in the 3d pH/carbonate saturation calculation (no transports, no biological transformations), and I moved it because I found it misleading that it was initialized in S/R dic_surfforcing_init.

I would recommend initialise all fields once in dic_init_varia.F, then read initial fields and/or the pickup file(s) (maybe also from dic_init_varia.F), and then do something with the initial fields if necessary in dic_ini_forcing.F, possibly combining it with dic_surfforcing_init.F

I think there is possibility to streamline initialization here, but perhaps for a future P/R?

maybe it is possible to reuse calcite_saturation.F instead of duplicating (most of) the code

Thats a good idea - I'll look into it.

jm-c · 2023-06-30T18:08:10Z

@mjlosch Thanks for the comments.
@seamanticscience I would suggest that we check together few things here and see what is the best way to finish this PR. Will email you to check when you are available. In the mean time, could wait a little bit before making new changes.

seamanticscience · 2023-06-30T19:38:48Z

@jm-c I made a few changes, but I will hold off committing them until we talk next week.

jm-c · 2023-07-07T14:01:23Z

@seamanticscience After we chat a bit about this PR, I took a look at how to make a single dic pickup file and realized that we would need to make some changes to the initialization routines (in line with what @mjlosch suggested):
If we want the 3D pH to be initialized outside dic_surfforcing_init.F (e.g., a more logical place would be in dic_ini_forcing.F or dic_init_varia.F), we cannot keep the call to DIC_READ_PICKUP there.

I could make these changes (i.e., initialization + single pickup file) in this PR. My current preference would be to call all dic initialization S/R from dic_init_varia.F instead of from gchem_init_vari.F).

However, we could also decide to have a second (short) PR with your first commit
(git hash: c062a96): "Fix up calcite dissolution code, with bugfix in Keir rates H/T @jahn"
which is independent of the 3D pH restart. There is also an inconsistent use of externForcingCycle + externForcingPeriod (in dic_surfforcing_init.F) vs DIC_forcingCycle + DIC_forcingPeriod (in dic_fields_load.F) that could also be fixed in this short PR (or in this PR).

@seamanticscience what do you think ?

seamanticscience · 2023-07-07T22:35:30Z

@jm-c OK, let's do a bugfix P/R and then tackle the broader issues of pkg/dic restarts in a separate P/R.

jm-c · 2023-09-19T15:54:34Z

Will keep this PR open but not work on it since:

PR pkg/DIC carbonate dissolution #759 contains the fix/improvement for carbonate dissolution (the first commit in this PR branch), so it is not needed here anymore.
the cleaning and reordering of pkg/dic initialisation (from PR Clean and fix pkg/dic initialisation #757) will facilitate the implementation of 3-D pH pickup,
so that it would be better to wait for 757 to be merged-in before revisiting this PR changes. At that time, could decide to
finish this PR (after merging updates) or move most of the changes here to a new PR.

seamanticscience · 2024-01-23T18:36:47Z

Picking up the baton from @jm-c (#757) 🏃‍♂️💨

seamanticscience closed this Apr 6, 2023

seamanticscience force-pushed the dic_silicate_mods branch from a7cfa71 to ab47de6 Compare April 6, 2023 19:17

seamanticscience reopened this Apr 6, 2023

seamanticscience marked this pull request as ready for review April 6, 2023 21:06

mjlosch reviewed Jun 22, 2023

View reviewed changes

jm-c mentioned this pull request Aug 3, 2023

Clean and fix pkg/dic initialisation #757

Merged

seamanticscience mentioned this pull request Aug 10, 2023

pkg DIC using 3-D calcite-saturation: wish list #678

Open

jm-c added the wontfix This will not be worked on label Sep 19, 2023

seamanticscience closed this Jan 23, 2024

seamanticscience force-pushed the dic_silicate_mods branch from bf733b8 to d4b64b2 Compare January 23, 2024 17:57

Read 3D pH pickup and check for cold start in calcite_saturation.F

58648c5

seamanticscience reopened this Jan 23, 2024

jm-c added work in progress Should not be merged until this label is removed and removed wontfix This will not be worked on labels Jan 24, 2024

seamanticscience added 2 commits March 5, 2024 11:05

Use pH3D variable

f36be27

initialize pH3D variable

9af8de9

jm-c linked an issue Apr 22, 2024 that may be closed by this pull request

pkg DIC using 3-D calcite-saturation: wish list #678

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3D pH pickup for pkg/DIC calcite saturation #718

3D pH pickup for pkg/DIC calcite saturation #718

seamanticscience commented Apr 5, 2023

mjlosch left a comment

mjlosch Jun 22, 2023

seamanticscience Jun 30, 2023

mjlosch Jun 22, 2023

mjlosch Jun 22, 2023

seamanticscience commented Jun 30, 2023

jm-c commented Jun 30, 2023

seamanticscience commented Jun 30, 2023

jm-c commented Jul 7, 2023

seamanticscience commented Jul 7, 2023

jm-c commented Sep 19, 2023

seamanticscience commented Jan 23, 2024 •

edited

3D pH pickup for pkg/DIC calcite saturation #718

Are you sure you want to change the base?

3D pH pickup for pkg/DIC calcite saturation #718

Conversation

seamanticscience commented Apr 5, 2023

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

mjlosch left a comment

Choose a reason for hiding this comment

mjlosch Jun 22, 2023

Choose a reason for hiding this comment

seamanticscience Jun 30, 2023

Choose a reason for hiding this comment

mjlosch Jun 22, 2023

Choose a reason for hiding this comment

mjlosch Jun 22, 2023

Choose a reason for hiding this comment

seamanticscience commented Jun 30, 2023

jm-c commented Jun 30, 2023

seamanticscience commented Jun 30, 2023

jm-c commented Jul 7, 2023

seamanticscience commented Jul 7, 2023

jm-c commented Sep 19, 2023

seamanticscience commented Jan 23, 2024 • edited

Suggested addition to `tag-index`

seamanticscience commented Jan 23, 2024 •

edited