-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
3D pH pickup for pkg/DIC calcite saturation #718
base: master
Are you sure you want to change the base?
3D pH pickup for pkg/DIC calcite saturation #718
Conversation
a7cfa71
to
ab47de6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
anddic_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 indic_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 indic_ini_forcing.F
, possibly combining it withdic_surfforcing_init.F
- maybe it is possible to reuse
calcite_saturation.F
instead of duplicating (most of) the code
pkg/dic/dic_ini_forcing.F
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/dic/dic_calcite_read_pickup.F
Outdated
INTEGER length_of_rec | ||
_RL vec(1-Olx:sNx+Olx,1-Oly:sNy+Oly,Nr,nSx,nSy) | ||
CEOP | ||
#ifdef DIC_CALCITE_SAT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
pkg/dic/dic_ini_forcing.F
Outdated
#endif | ||
C- end bi,bj loops | ||
C Calculate the inorganic carbon sytem disociation coefficients at the surface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
Thanks for your comments @mjlosch
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
Right, that is partly me being over-cautious. The
I think there is possibility to streamline initialization here, but perhaps for a future P/R?
Thats a good idea - I'll look into it. |
@mjlosch Thanks for the comments. |
@jm-c I made a few changes, but I will hold off committing them until we talk next week. |
@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): 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 However, we could also decide to have a second (short) PR with your first commit @seamanticscience what do you think ? |
@jm-c OK, let's do a bugfix P/R and then tackle the broader issues of |
Will keep this PR open but not work on it since:
|
bf733b8
to
d4b64b2
Compare
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
toS/R DIC_INI_FORCING
.Suggested addition to
tag-index
o 3D pH pickup for calcite saturation calculation