NorESMhub / BLOM

Bergen Layered Ocean Model
GNU Lesser General Public License v3.0
16 stars 25 forks source link

Merge master into beyond-CMIP6 branch #311

Closed jmaerz closed 7 months ago

jmaerz commented 7 months ago

This is the PR to merge master into the feature-hamocc_beyond-CMIP6 branch in order to update the beyond-CMIP6 branch with recent reformatting and reorganization in master.

In support for this merge (and due to the numerous changes carried out in master wrt the current beyond-CMIP6 branch, I opened a draft PR #312 for visualizing real changes/bug fixes carried out in the beyond-CMIP6 branch. (My branch mgmaster2beyCMIP6 was checked out from the most recent beyond-CMIP6 branch and I merged the current master into it - so the difference between mgmaster2beyCMIP6 and the current master highlight real bug fixes.)

Things still to do after this PR to get a version ready for NorESM2.1:

For NorESM2.3 (or2.x):

The above mentioned small changes, I would do via a new PR (likely on Monday).

jmaerz commented 7 months ago

@TomasTorsvik and @JorgSchwinger , I was setting up a first regression test wrt the feature-hamocc_beyond-CMIP6 branch (testcase: SMS_D_Ld1.T62_tn14.NOINYOC.betzy_intel). As before, I used the regression testing, while then (due to unresolved issues with the cprnc), I checked manually via cdo -b F64 -sub .... From cprnc, I just received two expected failures - wrt the new namelist (the comparison failed, since new parameters were found, OCN_CO2_TYPE and THRH_OMEGAA, and a new namelist was introduced, config_bgc - both expected) and the check of model outcomes (due to lacking dependencies in cprnc). Manually, I received bit-identical model results. So from my side, I perceive this as ready to be merged, while I would also be happy to see somebody else testing it again. But I leave this decision to you and wait for your approval (with or without additional testing).

With respect to the new tagged 2.1 version, there are two more thing left to do:

These changes will be done via a new PR afterward.

JorgSchwinger commented 7 months ago

With respect to the new tagged 2.1 version, there are two more thing left to do:

* change the silicate dissolution rate  (@JorgSchwinger : which one to use?)

* potentially 'fix' the unpleasant writing of code wrt the parameters `bifr13` and `bifr14`.

These changes will be done via a new PR afterward.

Regarding the dissolution rate: I am wondering - if the aim is really only a bug-fixed version of NorESM2, then it would be consequent to leave the rate as is (and NOT use the sediment spinup). This will be closest to NorESM2 behavior. We would then fix the dissolution rate afterwards.

Regarding bifr13/14 - I agree this isn't nice, we could change it to bifr13_ini and bifr14_ini in mo_param_bgc (which are then used to initialize the fields in aufr_bgc and mo_ini_fields) and use bifr13/14 as a local variable in ocprod?

JorgSchwinger commented 7 months ago

BTW, there is also no need to have the variable bifr13_perm defined in mo_biomod - this is used only as a local variable in ocprod

jmaerz commented 7 months ago

Hi @JorgSchwinger , I am uncertain, about the Si-dissolution rate - I understand that it isn't a real bug, but I personally would opt for replacing it with the proper value - also to have a tagged version to which one can easily refer to when speaking about the N-cycle, etc. - but maybe @TomasTorsvik has a different opinion (what we also can do is to bring in the updated dissolution rate soon as another tagged version on the way to NorESM 2.3 (or 2.x) - which would also accommodate including the N-cycle - I anyway would like to merge all these developments soon together into master as a new tagged version). For now, I would like to prepare the beyond-CMIP6 branch to be merged back to master in preparation for NorESM2.1 (with whichever value for the rate).

Wrt bifr13, etc. I'll have a closer look once the stuff is merged, but I would expect that we can proceed as you suggested (there are/might be a few more of those issues, though). I will merge this PR soon.

JorgSchwinger commented 7 months ago

My understanding was that we would have 3 releases:

From this logic I would see the dissolution + sediment spinup in CMIP6 +. The problem is that changing the dissolution rate only, will change DMS emission quite a bit, while the sediment spinup compensates for it to a large degree. (But sediment spinup is clearly not a bug fix)

jmaerz commented 7 months ago

Hi @JorgSchwinger and @TomasTorsvik , I am really sorry, but since I knew that the preparation was a bit in a rush, I went through the changes again and had to fix two more things that escaped during the merging - I'll do another test run and if that works out as expected, I'll merge. (Not sure, why the github testing now fails in two tests ... - this looks more like an issue on the github side?!)

Wrt to your last comment

* bug fixed CMIP6

* CMIP6 + (but still the old coupling framework)

* NorESM2.5 with nupoc and updates that rely on nuopcy

From this logic I would see the dissolution + sediment spinup in CMIP6 +. The problem is that changing the dissolution rate only, will change DMS emission quite a bit, while the sediment spinup compensates for it to a large degree. (But sediment spinup is clearly not a bug fix)

I am not entirely certain, if I understand clearly, what you mean by CMP6 + - do you refer to NorESM2.3 by it? But in any case, I am happy to postpone adjusting the Si-dissolution rate to a later time point (this isn't a big code change anyway - I guess, I would then opt for soon moving on to some tag for 2.3 - but this is upon later discussion).

jmaerz commented 7 months ago

Dear @JorgSchwinger and @TomasTorsvik , after performing another regression test run against the beyond-CMIP6 branch, I go ahead with the merge and try to fix the bifrxx issue afterward.

mvertens commented 7 months ago

@jmaerz - this is my understanding:

noresm2_1: bug fixed CMIP6
noresm2_3: new infrastructure changes for oslo-aero and incorporation of changes for key-clim and FORCES experiments 
noresm2_5:  nuopc infrastructure and deprecation of MCT
jmaerz commented 7 months ago

@mvertens , thanks for the clarification/comment!