NorESMhub / BLOM

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

refactor buildnml to use the CIME-CCS python based namelist generation capabilities #263

Closed mvertens closed 10 months ago

mvertens commented 11 months ago

This PR refactors the namelist generation in BLOM to use the python based capabilities in CIME. This is the way that CICE, all CDEPS components, CMEPS, WW3DEV and POP generate their namelists. CAM and CTSM still use an older perl based scripting with the intention of moving away from that in the future.

  1. buildnml is now a python based script
  2. there is a new file namelist_generation_blom.xml that is used to generate all of the namelists that are used by blom. The matching functionality is based on the compset, resolution as well as values in a new python dictionary config[] that is introduced in the new buildnml. Entries in this xml are of the following forms:

for real variables:

  <entry id="mdv2hi">
    <type>real</type>
    <category>limits</category>
    <group>limits</group>
    <values>
      <value>.02</value>
      <value blom_unit="cgs">2.</value>
      <value ocn_grid="tnx0.25v4"  blom_unit="cgs">.15</value>
      <value ocn_grid="tnx0.25v4"  blom_unit="mks">.0015</value>
      <value ocn_grid="tnx0.125v4" blom_unit="cgs">.1</value>
      <value ocn_grid="tnx0.125v4" blom_unit="mks">.001</value>
    </values>
    <desc></desc>
  </entry>

for file based variables:

  <entry id="tdfile">
    <type>char</type>
    <category>limits</category>
    <group>limits</group>
    <values>
      <value>UNSET</value>
      <value ocn_grid="tnx2v1">$DIN_LOC_ROOT/ocn/blom/bndcon/tidal_dissipation_tnx2v1_20130419.nc</value>
      <value ocn_grid="tnx1v4">$DIN_LOC_ROOT/ocn/blom/bndcon/tidal_dissipation_tnx1v4_20170605.nc</value>
      <value ocn_grid="tnx0.25v4">$DIN_LOC_ROOT/ocn/blom/bndcon/tidal_dissipation_tnx0.25v4_20170626.nc</value>
      <value ocn_grid="tnx0.125v4">$DIN_LOC_ROOT/ocn/blom/bndcon/tidal_dissipation_tnx0.125v4_20221013.nc</value>
    </values>
    <input_pathname>abs</input_pathname>
    <desc>
      Name of file containing tidal wave energy dissipation divided by
      by bottom buoyancy frequency
    </desc>
  </entry>

Note that <input_pathname>abs</input_pathname> is used to create the Buildconf/blom.input_data_list entries.

for character variables:

  <entry id="swamth">
    <type>char</type>
    <category>limits</category>
    <group>limits</group>
    <values>
      <value>jerlov</value>
    </values>
    <desc></desc>
  </entry>
  1. the Buildconf/blom.input_data_list is automatically created based on the entrires in namelist_generation_blom.xml.
  2. user settings of namelist values can now be done in user_nl_blom via keyword/value settings. This is a big advantage of the current implementation. In addition, checks are made on the user entries in user_nl_blom (i.e. if an integer is expected, a logical cannot be entered, etc).
  3. documentation of all namelist variables is now in ocn_in.readme that is placed along side the generated ocn_in namelist in $RUNDIR.
  4. the older csh based buildnml (without the namelist documentation) is being temporarily kept as a renamed file - buildnml.csh.
  5. this refactor is a critical part of getting regression testing working in BLOM. The reason is that in general daily history files need to be compared both for ecosys and non-ecosys configurations. For comparisons to work - the fields need to be at least single precision. In addition for performance tests - you don't want to have any history files created - hence the empty_hist attribute below. With this refactor - this is easily done via the following type of xml entry:
<entry id="srf_phosph">
    <type>integer(3)</type>
    <category>diabgc</category>
    <group>diabgc</group>
    <values>
      <value>0,2,2</value>
      <value is_test="yes">4,2,2</value>
      <value is_test="yes" empty_hist="yes">0,0,0</value>
    </values>
    <desc>Phosphorus (po4) [mol P m-3]</desc>
 </entry>

Testing: Used the following create_newcase command out of this code sandbox and a code sandbox that had the previous BLOM namelist generation:

./create_newcase --case <CASENAME> --compset 2000_SATM_SLND_SICE_BLOM%ECO_SROF_SGLC_SWAV_SESP --res T62_tn14 --run-unsupported --project <PROJECT> --mach betzy

For the following different grids verified that the sorted namelists were the same

xmlchange OCN_GRID=tnx2v1
xmlchange OCN_GRID=tnx1v4
xmlchange OCN_GRID=tnx0.25v4
xmlchange  OCN_GRID=tnx0.125v4

For tnx1v4 verified that the sorted namelists were the same:

xmlchange OCN_NCPL=48
xmlchange  BLOM_UNITS=mks
xmlchange BLOM_COUPLING=partial
xmlchange BLOM_COUPLING= partial and BLOM_VCOORD=cntiso_hybrid
xmlchange BLOM_TRACER_MODULE=iage
xmlchange BLOM_NDEP_SCENARIO=2000
xmlchange BLOM_NDEP_SCENARIO=hist
xmlchange BLOM_NDEP_SCENARIO=ssp245
TomasTorsvik commented 11 months ago

@mvertens - I tried to set up a NOINYOC run using a run script for noresm2.0.6, but based on the noresm_develop_v7_proto branch for NorESM and this feature/refactor_buildnml branch. Basically, I wanted to check if it was possible to run the same setup as before once we merge this PR into master.

What I find is that the grid T62_tn21 is not defined, and for grid T62_tn14 I get an error that seems to be related to a missmatch in the processor count (see below). I might be doing something wrong in my setup, but it would be good to ensure that we can still run CMIP6 compsets after merging this PR.

/cluster/projects/nn2980k/tomast/NORESM/NorESM-mvertens/components/blom//cime_config/buildlib:6: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import os, shutil, sys, glob, imp
ERROR: Command /cluster/projects/nn2980k/tomast/NORESM/NorESM-mvertens/components/blom/cime_config/buildlib_2.2 /cluster/projects/nn2980k/tomast/NORESM/cases/NOINYOC_T62_tn14_NorESM-mvertens_1y_20230803T1206 /cluster/work/users/toma
st/noresm/NOINYOC_T62_tn14_NorESM-mvertens_1y_20230803T1206/bld/lib /cluster/work/users/tomast/noresm/NOINYOC_T62_tn14_NorESM-mvertens_1y_20230803T1206/bld/ocn/obj failed rc=1
out=...calling blom buildcpp to set build time options
err=/cluster/projects/nn2980k/tomast/NORESM/NorESM-mvertens/components/blom/cime_config/buildlib_2.2:6: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import os, shutil, sys, glob, imp
ERROR: Command /cluster/projects/nn2980k/tomast/NORESM/NorESM-mvertens/components/blom/bld/blom_dimensions -n 1 -k 53 -d /cluster/projects/nn2980k/tomast/NORESM/NorESM-mvertens/components/blom/bld/tnx1v4 failed rc=1
out=
err=blom_dimensions: Cannot find patch.input file for 1 processors!
blom_dimensions: Available processor counts: 32 42 63 77 91 123 156 186 256 354
mvertens commented 11 months ago

@TomasTorsvik - we need to add the tn21 grid to ccs_config and also create a mesh for it. I'm happy to do that. Can you please change your permissions on /cluster/projects/nn2980k/tomast/NORESM/cases/NOINYOC_T62_tn14_NorESM-mvertens_1y_20230803T1206 so that I can see your directory - or have me added to nn2980k? That would be very helpful.

mvertens commented 11 months ago

@TomasTorsvik - you need to use noresm_develop_v7 in order to get the right proessor layout in blom.

I was successfully able to do the following:

> git clone https://github.com/NorESMhub/NorESM.git
> cd NorESM
> git checkout noresm_develop_v7
> ./manage_externals/checkout_externals -v
> cd components/blom
> git remote add mvertens https://github.com/mvertens/BLOM
> git fetch mvertens
> git checkout feature/buildnml_refactor
> cd ../../cime/scripts
> ./create_newcase --case NOINYOC_refactor --compset NOINYOC --res T62_tn14 --project nn9560k --run-unsupported --mach betzy
> cd NOINYOC_refactor
> ./pelayout (this provides a valid pelayout)
> ./case.build (this built the model with no problem)

So I need to add the T62_tn21 and an accompanying test in blom. Should I add any other resolution?

TomasTorsvik commented 11 months ago

@TomasTorsvik - you need to use noresm_develop_v7 in order to get the right proessor layout in blom.

I was successfully able to do the following:

> git clone https://github.com/NorESMhub/NorESM.git
> cd NorESM
> git checkout noresm_develop_v7
> ./manage_externals/checkout_externals -v
> cd components/blom
> git remote add mvertens https://github.com/mvertens/BLOM
> git fetch mvertens
> git checkout feature/buildnml_refactor
> cd ../../cime/scripts
> ./create_newcase --case NOINYOC_refactor --compset NOINYOC --res T62_tn14 --project nn9560k --run-unsupported --mach betzy
> cd NOINYOC_refactor
> ./pelayout (this provides a valid pelayout)
> ./case.build (this built the model with no problem)

So I need to add the T62_tn21 and an accompanying test in blom. Should I add any other resolution?

@mvertens - Thanks for the update, I will try your setup.

We often use T62_tn21 for testing, so that would be nice to have. We also use TL319_tn14 for OMIP-2 runs. In addition to T62_tn14, those are the main ones used for OMIP-type runs.

mvertens commented 11 months ago

@TomasTorsvik - I need to create an ESMF mesh file for tnx2v1 - and to do that I need a SCRIP file to start with. In searching /cluster/shared/noresm/inputdata it seems that the SCRIP file I need isremap_grid_tnx2v1_20130206.nc - but I cannot find it there. Can you point me to where I can find a valid SCRIP file for this resolution? I can then get everything else working. I have verified that TL319_tn14 works out of the box.

TomasTorsvik commented 11 months ago

@TomasTorsvik - I need to create an ESMF mesh file for tnx2v1 - and to do that I need a SCRIP file to start with. In searching /cluster/shared/noresm/inputdata it seems that the SCRIP file I need isremap_grid_tnx2v1_20130206.nc - but I cannot find it there. Can you point me to where I can find a valid SCRIP file for this resolution? I can then get everything else working. I have verified that TL319_tn14 works out of the box.

@mvertens - Sorry, I didn't work with grid generation, and I don't know where the remap file should be located. @matsbn , @JorgSchwinger - do you know where this file can be found?

In looking at /cluster/shared/noresm/inputdata/cpl/cpl6/map_tnx1v2_to_fv1.9x2.5_aave_da_140828.nc - I see the following:

// global attributes:
        :title = "ESMF Offline Regridding Weight Generator (the circles of latitude of domain_b are subdivided 8 times)" ;
        :normalization = "destarea" ;
        :map_method = "Conservative remapping" ;
        :conventions = "NCAR-CSM" ;
        :domain_a = "remap_grid_tnx1v2_20140828.nc" ;
        :domain_b = "remap_grid_fv1.9x2.5_20111104.nc" ;
        :grid_file_src = "remap_grid_tnx1v2_20140828.nc" ;
        :grid_file_dst = "remap_subgrid_grid_fv1.9x2.5.nc" ;
        :CVS_revision = "5.2.0rp2" ;
        :history = "Fri Aug 29 16:35:10 2014: ncap2 -O -s defdim(\"ni_a\",360);defdim(\"nj_a\",384);defdim(\"ni_b\",144);defdim(\"nj_b\",96) map_tnx1v2_to_fv1.9x2.5_aave_da_140828.nc map_tnx1v2_to_fv1.9x2.5_aave_da_140828.nc" ;
        :nco_openmp_thread_number = 1 ;
        :Created_by = "jschwing, Fri Aug 29 16:35:13 CEST 2014" ;
        :\1D_grid_indexing = "if n is 1D index, i runs fast, j runs slow: n=(j-1)*fast_grid_dim+i" ;
}

I've tried to independently create the scrip file using new NCO functionality - using the following: ncks --rgr infer --rgr scrip=scrip.nc domain.ocn.tnx1v2.140828.nc foo.nc but am getting the following warnings:

ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=132839, (lat,lon)_idx=(368,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=133199, (lat,lon)_idx=(369,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=133559, (lat,lon)_idx=(370,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=133919, (lat,lon)_idx=(371,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=134279, (lat,lon)_idx=(372,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=134639, (lat,lon)_idx=(373,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=134999, (lat,lon)_idx=(374,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=135359, (lat,lon)_idx=(375,359), (lat,lon) = (-79.9418, 249.5)
ncks: nco_grd_nfr() WARNING reports non-CCW gridcell at idx=135719, (lat,lon)_idx=(376,359), (lat,lon) = (-79.9418, 249.5)

CCW is counter clockwise rotation. So I am not comfortable trying to use the generated scrip file from the above.

JorgSchwinger commented 11 months ago

The tnx2v1 grid was generated by @IngoBethke, maybe he knows whether and where the file remap_grid_tnx2v1_20130206.nc is available?

matsbn commented 11 months ago

Thanks for the initiative which I believe is a very useful capability! I'm still on vacation and will not be able look at this carefully before next week. Some general comments: We started on a json based namelist generation in https://github.com/NorESMhub/BLOM/pull/83, but progress has been slow. The idea was to have a fairly similar namelist generation for standalone BLOM and for when running within NorESM. It would of course be great if this xml based namelist generation would be able to work in standalone mode. Also, BLOM master is currently working for all NorESM versions, so that compatibility would useful to check.

mvertens commented 11 months ago

@matsbn - thanks for your thoughts on this. You bring up some really good points. I totally agree about verifying that this should work for older versions of BLOM. If you could point me to how to get the code and what configurations I should test I'd be happy to do that. My question is, however, would we be using this latest version of BLOM with the older NorESM code base? I had assumed that the developments we are putting into BLOM right now would only be run with the development versions of NorESM. But maybe that is a wrong assumption on my part. It would be good to discuss this at the BLOM core meeting next week.

That is a really good point about standalone BLOM and could be a real problem this PR. I am using CIME functionality to generate the namelist and stand-alone BLOM and this requires a CIME CASE to be generated. We can discuss more about this as well.

mvertens commented 11 months ago

Some good news. I have leveraged a new namelist generation capability that just was developed a little while ago (ParamGen) that I believe will enable the use of namelist generation for both NorESM BLOM and stand-alone BLOM. I am in the process of testing this and will push back to this PR. I can go over this at the core meeting on Wednesday.

TomasTorsvik commented 11 months ago

compare-noresm206_vs_noresm_develop_v7_SST

Comparing noresm2.0.6 and noresm_develop_v7 : 1st year average SST from NOINYOC run.

I ran a NOINYOC_T62_tn14 test run with the noresm2.0.6 and the noresm_develop_v7 + BLOM feature/refactor_buildnml branch. There is a notable cooling around the equator in develop_v7. Not sure what is going on here. Will try noresm_develop_v7 + BLOM master branch.

mvertens commented 11 months ago

@tomas - thanks for doing these runs! Below are a summary of some of the differences I am seeing between these two cases: Below: (< is new > is 2.0.6)

BLOM: one difference I see in the blom namelist (other than in the develop version the blom namelist has a lot of new values new parameter values like edanis, addalk, etc ) is

<   ATM_CO2 = 284.7
>   ATM_CO2 = 284.32

<   SWACLIMFILE = '/cluster/shared/noresm/inputdata/ocn/blom/bndcon/Annual_clim_swa_tnx1v4_20210415.nc'
>   SWACLIMFILE  = ''

Runoff:

The runoff file is different in the two cases
< /cluster/shared/noresm/inputdata/lnd/dlnd7/RX1/runoff.daitren.annual.20190226.nc
> /cluster/shared/noresm/inputdata/lnd/dlnd7/RX1/runoff.daitren.annual.090225.nc

These two files have significantly different maxima.

CICE:

< uses CICE6
> uses CICE5

One thing to try also is to simply use DICE%SSMI instead of CICE in both cases to remove any differences relative to the ice component and also have the runoff files be identical in both runs.

mvertens commented 11 months ago

one other difference is that -DBROMO is now on by default at tnx1v4 - whereas it was off in the 2.0.6 case unless you explicitly defined HAMOCC_VSLS=TRUE. But I don't think this would impact the surface temperature (do you agree?).

mvertens commented 11 months ago

I have pushed back new changes to this branch which now uses ParamGen. There is new directory components/blom/cime_config/ParamGen along with new files in cime_config: ocn_in_paramgen.py and case_dict.py.

ocn_in_paramgen.byis a wrapper to ParamGen/param_gen.py and no longer depends on any CIME functionality. Similarly, case_dict.py and paramgen.pyno longer depend on any CIME functionality.

To get this working with stand-alone BLOM independent of CIME a separate buildnml script will need to be written created to define the config dictionary (needed by namelist_definition_blom.xml) and the case_dict dictionary.

NOTE: you need to have a recent python version to run this. On betzy I have been using module load Python/3.10.8-GCCcore-12.2.0

TomasTorsvik commented 11 months ago

I did two more runs, so now I have tested (A) noresm2.0.6 (B) noresm2.0.6 + BLOM master (C) noresm_develop_v7 + BLOM master (D) noresm_develop_v7 + BLOM feature/refactor_buildnml

So with the current master we have diverged to some extent from CMIP6, but the physics seem to be equivalent to the reference run.

@mvertens - Thanks for your comments! I think it's very valuable to have an overview of major changes that have been introduced in the model. Maybe we should think of collecting this somewhere?

JorgSchwinger commented 11 months ago

@TomasTorsvik: I (just the other day) did a comparison A vs B and had bit-identical results, but there are a few updates that lead to changes in diagnostic variables. Maybe this is what you are seeing?

TomasTorsvik commented 11 months ago

It seems that the change in PE table introduced by PR #262 caused master to diverge. See update to issue #258 .

jmaerz commented 11 months ago

Dear @mvertens , I saw the last two commits. When starting to introduce the new namelist, we (@JorgSchwinger and @TomasTorsvik & me) discussed the namelist as an optional namelist. The idea was/is to have default iHAMOCC values build-in hard-coded into the Fortran code and only expert users being able to i) set and ii) modify the namelist values (and not writing them always to the input-nml). I am not yet familiar with the python-based xml-change setup for the namelist that you introduce with this PR, but to me, it currently looks as if you hard-code the default values into the xml-file, which isn't exactly what we had in mind initially. Maybe @JorgSchwinger and @TomasTorsvik could comment as well, but we initially wanted to refrain from providing default values through the namelist.

Further: I personally do not see a need to handle math operations via the xml in the above described use case... - but maybe @TomasTorsvik and @JorgSchwinger disagree.

mvertens commented 11 months ago

@jmaerz - thanks for looking at this! I was hoping we could talk about this exact issue. I could not find a way to write out an optional namelist unless there were values actually in it. I can try to fix that. However, having default values that appear in a namelist provides a more data driven approach to see what you might want to change. If you simply hard code the defaults in the code - then someone has to look at the code itself to see if they want to introduce a namelist change. By having the defaults appear as part of ocn_in - I think it becomes more evident as to what the model is using without the user having to look in the namelist and the code. If you want me to back this out - its not clear to me how we can incrementally add namelist values to this namelist. You need to get all the namelist variables in the group that you define. Do you want to have a call to talk about this?

Also - I just thought of a simple way to add an empty namelist and just comment out the additional namelist values that I created if you feel that would be better for now. Let me know what you think would be better.

jmaerz commented 11 months ago

Dear @mvertens , thanks for looking into it and the swift response. I guess, our initial intention was to differentiate a bit between the 'normal' and the 'expert' users - the latter looking into the actual code. But maybe let's see what @JorgSchwinger and @TomasTorsvik will say after your implementations. With PR #261 , we were simply inserting an empty nml into the namelist file (as you've seen).

mvertens commented 11 months ago

@jmaerz - I'm fine with whatever approach is viewed as best. I can implement a hard-code fix to generate an empty namelist for now if that is preferred - but we can still have the namelist_definition have the additional namelists - but we won't append the namelist group. That will result in minimal changes to what I have now and result in what you had envisioned initially.

mvertens commented 11 months ago

@jmaerz - after thinking some more it makes sense for me to have this PR do exactly what you had with the empty namelist and we can then extend it easily if we want to moving forwards.

JorgSchwinger commented 11 months ago

I'm not sure what is the best solution here. Yes, our initial intention was to 'hide' the model parameter namelist (used for model tuning) from non-expert users. But on the other hand, for physical BLOM (and also CAM, CLM, CICE) there is a bunch of parameters in the namelist that non-expert users shouldn't fiddle around with.

So, if it is difficult to omit these paramters from writing the namelist, for me it would be fine to include them with default values (a warning in a comment could be added maybe?).

jmaerz commented 11 months ago

Dear @mvertens , that sounds good to me - so you found a way to also make use of the -by default empty- nml when one wants to modify parameters through that nml? @JorgSchwinger to me, this sounds also fine - which would somewhat suggest to initialize (all?) parameters through the namelist at some point?! I guess, I would try to avoid confusion about where the initialization values of parameters come from...

mvertens commented 11 months ago

I've just pushed back a simple change that currently hard-wires an empty namelist(s). It will be very easy to turn on the defaults - since it will only mean changing a few lines of code in buildnml.

mvertens commented 10 months ago

@matsbn @TomasTorsvik @JorgSchwinger @jmaerz - some very good news. I have found the reason for the differences in SST that @TomasTorsvik ran into. The key is the value of the cmeps/cpl7 namelist flux_albav.

In cmeps we have by default.
flux_albav = .false.
and in cpl7 we have by default
flux_albav = .true.

Note that to change the value in cmeps you need to do it via an xml variable: ./xmlchange CPL_ALBAV=TRUE

What flux_albav does in the mediator is set the values of the albedo to constant values. It was decided in CESM that if the ocean coupling would be hourly - then we should set flux_albav to .false. by default.

   if (flux_albav) then
       do n = 1,lsize
          ocnalb%anidr(n) = albdir
          ocnalb%avsdr(n) = albdir
          ocnalb%anidf(n) = albdif
          ocnalb%avsdf(n) = albdif
       end do
    else
       ! Solar declination
       ! Will only do albedo calculation if nextsw_cday is not -1.

       if (nextsw_cday >= -0.5_r8) then
          call shr_orb_decl(nextsw_cday, eccen, mvelpp,lambm0, obliqr, delta, eccf)

          ! Compute albedos
          do n = 1,lsize
             rlat = const_deg2rad * ocnalb%lats(n)
             rlon = const_deg2rad * ocnalb%lons(n)
             cosz = shr_orb_cosz( nextsw_cday, rlat, rlon, delta )
             if (cosz  >  0.0_r8) then !--- sun hit --
                ocnalb%anidr(n) = (.026_r8/(cosz**1.7_r8 + 0.065_r8)) +   &
                                  (.150_r8*(cosz         - 0.100_r8 ) *   &
                                  (cosz - 0.500_r8 ) * (cosz - 1.000_r8 )  )
                ocnalb%avsdr(n) = ocnalb%anidr(n)
                ocnalb%anidf(n) = albdif
                ocnalb%avsdf(n) = albdif
             else !--- dark side of earth ---
                ocnalb%anidr(n) = 1.0_r8
                ocnalb%avsdr(n) = 1.0_r8
                ocnalb%anidf(n) = 1.0_r8
                ocnalb%avsdf(n) = 1.0_r8
             end if
          end do
          update_alb = .true.

       endif    ! nextsw_cday
    end if   ! flux_albav

Using the code in this PR to do my comparison, I set flux_albav = .true. in the CMEPS run and the results look very similar to the cpl7 run.

@TomasTorsvik - can you please make this change in your nuopc run and try doing the yearly run again. Things should look okay. @TomasTorsvik @matsbn @JorgSchwinger @jmaerz - what default value do we actually want to have for flux_albav moving forwards - given that by default we are now coupling BLOM at an hourly frequency.

TomasTorsvik commented 10 months ago

comp_old-ref-new

SST output after 1 year: left: develop_v7 with CPL_ALBAV=FALSE ; center: noresm2.0.6 + BLOM master ; right: develop_v7 with CPL_ALBAV=TRUE

@mvertens - Thanks! This seems to solve the problem with the SST.

mvertens commented 10 months ago

@TomasTorsvik - thanks so much for confirming this! Moving forwards what do we want to set CPL_ALBAV for noresm? Do we want to adopt the same default as is used in CESM?

TomasTorsvik commented 10 months ago

@TomasTorsvik - thanks so much for confirming this! Moving forwards what do we want to set CPL_ALBAV for noresm? Do we want to adopt the same default as is used in CESM?

I think this may be a topic that we should discuss in a wider NorESM forum, although in this case it affects an OMIP-type run. My view for noresm development would be to use the same defaults as CESM, until at some point we need to tune the model,

mvertens commented 10 months ago

@matsbn @TomasTorsvik @jmaerz - I was wondering if we could move forwards with merging this tag now given that we have a release branch. I have made progress with the tnx2v1 grid - but it turns out that cice6 does not support that resolution - so getting that in will be more involved than I had expected. As a result - I would like to keep that out of this PR. I have generated our first set of baselines in /cluster/shared/noresm/noresm_baselines/.

mvertens commented 10 months ago

Just as another note - I need to create a new noresm v8 code base so that we can start new coupled model simulations and I'd like to use blom with the new namelist changes as part of that. I would also use this as a new step in migrating the new code from @jmaerz into the nuopc code.

jmaerz commented 10 months ago

Dear @mvertens , could you elaborate a bit on what you mean with this migration step of the extended nitrogen cycle? Is there a chance to test, whether the current master of BLOM is still usable with the MCT coupler (anyone willing to do this?)? - if it is, I would suggest to merge the master into the feature-hamocc_beyond-CMIP6 branch (#268) to update that one with the new iHAMOCC-namelist option which I then could merge into the extended nitrogen cycle branch - and then eventually merge the extended nitrogen cycle branch into the feature-hamocc_beyond-CMIP6 branch (#269), which would make life easier and less spread out - also for the nuopc preparation @TomasTorsvik , @JorgSchwinger , @mvertens , what do you think about it? - I could prepare all this early next week.

mvertens commented 10 months ago

@jmaerz - thanks for laying out a plan. I am confused as to when this PR could then get merged into master. Are you saying that you would like to have all of your PRs merged to master (i.e. #268 and #269) BEFORE we bring in this PR? Maybe a quick chat would be very helpful at this point. Would you have time this afternoon?

jmaerz commented 10 months ago

I would have time, yes. However. I fear I am not exactly the right person to decide the steps. My point is that I would like to avoid double work with all the merging processes. My take from the last BLOM/NorESM development meeting with @matsbn was that with the new release branch (#267), we can break CMIP6 backwards compatibility when it comes to scientific results, however, being still able to run NorESM with the MCT coupler was some wish/requirement that @matsbn suggested (if I got it right - please correct me, if I am wrong). My comment is NOT affecting the merging of this PR into master, it was more directed to your comment about making the extended nitrogen cycle ready for nuopc, while still being able to do all the coupling as of NorESM as laid out in https://github.com/NorESMhub/BLOM/issues/147#issuecomment-1671874451. I guess, I am still having issues to fully understand the consequences of your developments in BLOM on coupling abilities with different versions of NorESM...

As a brief information: thus far, from the iHAMOCC point of view, any developments in master aimed at scientific results backward compatibility with the CMIP6 version. Bug fixes that affected this backward compatibility were pushed into feature-hamocc_beyond-CMIP6 (or in short: beyond-CMIP6). We continuously merged developments done in master also into the beyond-CMIP6 branch to keep that up-to-date with master. My extended nitrogen cycle branch has these bug fixes from the beyond-CMIP6 branch and will eventually merged into the beyond-CMIP6 branch and bringing all this into master at some point. This does NOT affect your developments in master though (since these will be merged further into beyond-CMIP6). Care needs to be taken, once we introduce the #264 PR, since some of your code re-writings will potentially affect the bug fixes we did in the beyond-CMIP6 branch, but this is something we will be taking care of.

mvertens commented 10 months ago

@jmaerz - thanks for your clarifications. I would definitely expect the new release branch to run with mct - but not necessarily the changes going into master (at least they would not be routinely tested). Sorry if my comment was confusing about the extended nitrogen cycle. I'd like to migrate the changes you needed to put into the mct cap and the mct coupler for your changes to the extended nitrogen cycle into the blom nuopc cap and cmeps. I would view a good starting point for that as being master once this PR is accepted.

mvertens commented 10 months ago

I am also happy to defer the #264 PR until after we get your changes migrated into the NUOPC cap and CMEPS.

matsbn commented 10 months ago

I tried this PR using NorESM2.0.6 (tag release-noresm2.0.6). I got the following error during case.setup:

  File "/cluster/projects/nn9560k/matsbn/GitHub/matsbn/NorESM2.0.6/components/blom//cime_config/buildnml", line 20, in <module>
    from CIME.Tools.standard_script_setup import *
ModuleNotFoundError: No module named 'CIME'

Other CIME modules seem to load fine using NorESM2.0.6. Do you think it is possible to overcome this issue @mvertens?

mvertens commented 10 months ago

@matsbn - thanks for trying this out. Yes - I can find a way around this I think.

mvertens commented 10 months ago

@matsbn - I'm pushing things back but its not quite working yet. I'll let you know when I have things working.

matsbn commented 10 months ago

@matsbn - I'm pushing things back but its not quite working yet. I'll let you know when I have things working.

Great that you are having a look at this!

mvertens commented 10 months ago

I have gotten it working in terms of the namelist generation and build - but for some reason I submit it to the queue and it starts up and then immediately dies. No log file or anything. I'd like to duplicate exactly what you did. What create_newcase command did you use out of the release code (release-noresm2.0.6)?

mvertens commented 10 months ago

@matsbn - I have pushed a fixes back that now enables buildnml work with the older version of CIME that is in the release code with blom replaced by this PR. ./create_newcase --case /cluster/home/mvertens/noresm/test_blom_newnl --compset NOINY --res T62_tn14 --project nn9560k --mach betzy I also ran the original checkout and verified that everything was bit-for-bit. Hopefully, you can confirm that this also works for you.

TomasTorsvik commented 10 months ago

@mvertens , @matsbn - thanks to both of you for testing this PR. I will meet with Jöran and Jörg today, hopefully we can move forward with this.

mvertens commented 10 months ago

@TomasTorsvik @jmaerz @mats - thanks all for your careful review of this PR. I think that it will make new regression testing much easier moving forwards.