NorESMhub / BLOM

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

NorESM is no longer bfb identical after merging PR-280 #285

Closed TomasTorsvik closed 8 months ago

TomasTorsvik commented 8 months ago

Merging PR #280 seems to have caused a change that breaks bfb reproducible results with earlier model versions. This was not an expected change from merging the PR.

jmaerz commented 8 months ago

Hi @JorgSchwinger , I won't have time to look into it until Friday, but maybe could you tell, if it is only the carbon isotopes or other tracers as well?

TomasTorsvik commented 8 months ago

I get bfb backwards compatible output for a standard 1 year NOINYOC_T62_tn21 run.

JorgSchwinger commented 8 months ago

To clarify, other tracers than those related to C-isotopes are still bfb. I have been running with C-isotopes and sediment activated.

@mvertens my case directory is here:

/cluster/projects/nn2345k/schwinger/cases/NOINYOC_T62_tn21_27_test_2x1

mvertens commented 8 months ago

@JorgSchwinger - thanks. I've been staring at the differences in the ocn log files - and it appears that no differences come in until time step 657557 (the run started at 657001) - that's a bit past 23 days into the run. I've been looking at the code - and its not clear to me what can trigger this after 23 days. Any suggestions as to where to look further?

<       0.0611  sec for step   657557
>       0.0613  sec for step   657557
4422c4422
<  No.            8     11465951666961.4          0.292337603760911
>  No.            8     11465951479028.5          0.292337598969346
4424c4424
<  No.            9   -2.884235613280459E+015     -73.5368988426710
>  No.            9   -2.884235613400504E+015     -73.5368988457317
4439c4439
<  Sediment No.            5     16471567453430.7
>  Sediment No.            5     16471567423524.1
4441c4441
<  Sediment No.            6   -2.653883035249488E+015
>  Sediment No.            6   -2.653883035734956E+015
JorgSchwinger commented 8 months ago

@mvertens, @jmaerz @TomasTorsvik Update:

1) it is the sediment - with C-isotopes and sediment-bypass activated it is bfb (which explains why your tests, Mariana, passed) 2) Including the sediment both, pre-#280 and post-#280 results, don't look right, so there could be some coding error in the C-isotope sediment that plays out differently when ifdefs are removed

image Results for 13C with sediment (looks wrong)

image Results for 13C without sediment

jmaerz commented 8 months ago

Hi @JorgSchwinger , the mapping should work correctly after shifting the init of the tracer indices, right? You're using both times master, just different versions, right - including already the fixes by Alok, I assume?

JorgSchwinger commented 8 months ago

@jmaerz what do you mean by "shifting the init of the tracer indices"? I'm using master for both runs, just at different commits

jmaerz commented 8 months ago

@JorgSchwinger - sorry - a bit short thought: I meant the change in mo_param1_bgc - now the initialization of the trc-indices called from somewhere in BLOM.

JorgSchwinger commented 8 months ago

I have put in a PR (#287) that fixes this. The issue was that during a restart from restart files without C-isotopes, the initialization of C-isotopes is done in aufr_bgc. This initialization uses observed d13C (not 13C), which has some advantages. However this wasn't done correctly in aufr_bgc, and so d13C was used directly for initialization of the sediment fields. This meas there have been negative values, and this is never checked and catched (for sediment fields). I don't know exactly where and why a divergence between the code with and without ifdefs came from, but I imagine that there might have been NaN or inf values occurring, which played out differently in the compiled code with and without ifdefs. Don't know if this makes sense.

mvertens commented 8 months ago

@JorgSchwinger - thanks so much for tracking this down! Are we okay then with moving to #281?