Skip to content
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

Adds icepack BGC #6457

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

njeffery
Copy link
Contributor

Changes are non-BFB with biogeochemistry and/or sea ice aerosols active.

Tested in ocean-sea ice BGC active simulations.

Needs to be tested and merged with main icepack-integration branch
BFB in the physics.
Changed back to original formulation.
-bugfix in carbon conservation - Fluxes scaled by aicen not aicen_init
-puts snow aerosols into ice rather than ocean for small snow volumes
-Changes vertical z-tracer flags so that aerosols and bgc are turned on separately:
 1) config_use_column_biogeochemistry indicates biogeochemical tracers are on
 2) config_use_zaerosols indicates dust and black carbon are on

BFB in the default configuration.  nonBFB with zaerosols or column_biogeochemistry
This column_package subroutine was accidentally added during the merge.
BFB
Most of the updates are for use in init_zbgc which now
initializes many of the configs, indices and size declarations
for bgc

Contains updates to column package to make
puny and accuracy consistent with carbon conservation check

BFB with bgc and aerosols off
Removed zsalinity references
BFB
…ated-icepack-bgc

This version uses a branch of Icepack, not main  (see .gitmodules)
Modifications bring changes to enable Icepack bgc into master
BFB with physics only.
Added flag to turn off BGC coupling between the ocean and sea ice:
config_couple_bgc_fields=false

Added black carbon conservation analysis member
-track ocean black carbon flux

Added carbon conservation check options
in interfaces for both icepack and column

Bugfixes to column package version:
1. Bugfixes to lateral melt BGC snow flux and ice flux
2. Bugfix to add_new_ice_bgc
-replaces adjust_tracer_profile with simpler and
more accurate version update_vertical_bio_tracers

Define carbon molar mass in mpas constants
Commented out problematic omp directives in icepack interface

Corrections to some bgc Registry descriptions (still more to do)

NOTE: Changed submodule path for Icepack to a branch that works with the BGC interface.
nonBFB with BGC
-Cleans up some missing bgc info in Registry  (more to do)
-Adds some additional info to carbon conservation analysis member
-Saves bgc ocean fluxes weighted by ice area (for carbon conservation)
-Comments out $omp statements around step therm1 (code was crashing
-with seg fault)
-Removes and cleans up unnecessary write statements for carbon debugging

Tested with E3SM-Project/Icepack.git branch njeffery/merge-fixes-to-bgc
commit c090b4b20e4ec0317d2c7d742c5ab291ec7a2240
BFB except correcting a seg fault
Now consistent with E3SM master
BFB
Small error introduced in last commit - broke compile
BFB
Merge remote-tracking branch 'remotes/origin/njeffery/seaice/updated-icepack-bgc'
into origin/master
-First uses init_parameters to initialize bgc namelist parameters
-Second (icepack_init_zbgc_tracer_indices) calls bgc indexing and array definitions.
-Third defines parameter arrays for bgc (init_zbgc)

bfb  (round-off level in bgc)
Adds 2 namelist fields:
config_use_atm_dust_file - true, use monthly dust climatology
config_use_iron_solubility_file -- true, use dust-iron solubility map from file
(in icepack: use_atm_dust_sol = .not.config_use_iron_solubility_file)

New fields track atmospheric wet and dry dust separately from the coupler to compute the
iron contribution.

Fraction of iron in dust is also generalized and read from a file.
Dust/iron file is identical to the one used by mpas-o.

Corrects some units in Registry.

BFB with new namelist parameters = false and for all runs without zaerosols or without iron.
This field oceanDustIronFlux will be used by the ocean for
dust-iron consistency across the coupled system

Also corrects a bug in the calculation of iron from dust introduced
in the last commit

Works with E3SM-Project/Icepack.git origin/fixes-to-zaerosols-take3
BFB with zaerosols off
-adds subroutine in the icepack interface,
init_zbgc_tracer_indices, to replace icepack_init_zbgc_tracer_indices.

-moves icepack_init_bgc_trcr from icepack to
init_bgc_tracer_indices in  mpas_seaice_icepack

Tested in GCASE with ice-ocean bgc and aerosols active

works with njeffery/Icepack.git  branch: fixes-to-bgc-indices
@njeffery njeffery added BGC mpas-seaice non-BFB PR makes roundoff changes to answers. labels May 31, 2024
Copy link

github-actions bot commented May 31, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6457/
on branch gh-pages at 2024-06-22 17:35 UTC

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial comments: I'd like to run some D tests on the code. Here are some things I noticed, just reading through the code diffs. Comments on mpas-seaice code are direct. For Icepack:

  • Where is this new submodule hash located (which fork/branch)? Is it different from the submodule listed in E3SM/.gitsubmodules?
  • Do the changes in icepack_aerosol.F90 change the answers for ‘old’ aerosol runs? e.g. is subroutine update_snow_bgc called when tr_aero=T?
  • icepack_parameters.F90: Is grid_o_t used in icepack? I had removed it from the parameters list because it wasn't used.
  • icepack_therm_itd.F90: The first initialization of vicen_init (originally =0, now =vicen(:)) in lateral_melt could be removed completely, since it’s set later. Or initialize both vicen_init and vsnon_init here and remove the later ones.
  • icedrv_init_column.F90: There’s a new cpp, NEWINITZBGC. Should that be here? I don't think it's set anywhere in the icepack driver.

FYI @apcraig

endif
endif

abortFlag = icepack_warnings_aborted()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep the abortFlag setting right after the call to icepack_step_therm2, to be consistent with the rest of the code. Having other non-icepack code in between does not change it.

!ratio_S2N_diatoms_in = config_ratio_S_to_N_diatoms, &
!ratio_S2N_sp_in = config_ratio_S_to_N_small_plankton, &
!ratio_S2N_phaeo_in = config_ratio_S_to_N_phaeocystis, &
!ratio_Fe2C_diatoms_in = config_ratio_Fe_to_C_diatoms, &
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of the bio parameters are commented out here. If they are being sent into icepack somewhere else, then remove them completely here. Could add a comment about if/when/where they are initialized in icepack.

!
!sw_redist_in = , & ! not yet implemented in MPAS-SI (stealth feature)
!sw_frac_in = , & ! not yet implemented in MPAS-SI (stealth feature)
!sw_dtemp_in = , & ! not yet implemented in MPAS-SI (stealth feature)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These sw_ parameters should not be changing in this PR.

config_use_vertical_biochemistry!, &
! config_use_vertical_zsalinity !echmod deprecate
config_use_vertical_tracers, &
config_use_vertical_zsalinity, &
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zsalinity option has been deprecated in Icepack, so should not be used here.

call icepack_init_bgc(&
ncat=nCategories, &
nblyr=nBioLayers, &
nilyr=nIceLayers, &
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't icepack already know what nilyr and ncat are by the time icepack_init_bgc is called?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the cleanup I was hoping to do with the bgc update. But first priority was to converge on the init_zbgc. I still would like to go thru the bgc interfaces carefully and identify things that could be removed and things that should be made optional.

@apcraig
Copy link
Contributor

apcraig commented Jun 1, 2024

Initial comments: I'd like to run some D tests on the code. Here are some things I noticed, just reading through the code diffs. Comments on mpas-seaice code are direct. For Icepack:

* Where is this new submodule hash located (which fork/branch)? Is it different from the submodule listed in E3SM/.gitsubmodules?

* Do the changes in icepack_aerosol.F90 change the answers for ‘old’ aerosol runs? e.g. is subroutine update_snow_bgc called when tr_aero=T?

* icepack_parameters.F90:  Is grid_o_t used in icepack?  I had removed it from the parameters list because it wasn't used.

* icepack_therm_itd.F90: The first initialization of vicen_init (originally =0, now =vicen(:)) in lateral_melt could be removed completely, since it’s set later.  Or initialize both vicen_init and vsnon_init here and remove the later ones.

* icedrv_init_column.F90: There’s a new cpp, NEWINITZBGC.  Should that be here?  I don't think it's set anywhere in the icepack driver.

FYI @apcraig

I'm testing the new icepack now too. The NEWINITZBGC was something I put in there as part of the transition/recommendation to show where I was hoping we would go. I am going to have a PR to NJ's Icepack branch to clean this up.

Also, I checked for grid_o_t. It's not used in Icepack and no longer appears in the Icepack driver or CICE.

@apcraig
Copy link
Contributor

apcraig commented Jun 1, 2024

@njeffery, has any testing been done with "debug" build flags. I'm running into several divide by zeros and other things are part of my testing of standalone Icepack. I will try to implement code fixes and then create a PR to your Icepack branch. Then we can discuss whether some of the fixes are due to incorrect setup of a case or other things.

@njeffery
Copy link
Contributor Author

njeffery commented Jun 3, 2024

@njeffery, has any testing been done with "debug" build flags. I'm running into several divide by zeros and other things are part of my testing of standalone Icepack. I will try to implement code fixes and then create a PR to your Icepack branch. Then we can discuss whether some of the fixes are due to incorrect setup of a case or other things.

Yes. I've compiled with debug. I'll test again was I've added some of Elizabeth's corrections.

@njeffery
Copy link
Contributor Author

njeffery commented Jun 3, 2024

@eclare108213 and @apcraig : The corresponding icepack branch is E3SM-Project/Icepack.git njeffery/fixes-to-bgc

@njeffery
Copy link
Contributor Author

njeffery commented Jun 3, 2024

Initial comments: I'd like to run some D tests on the code. Here are some things I noticed, just reading through the code diffs. Comments on mpas-seaice code are direct. For Icepack:

* Where is this new submodule hash located (which fork/branch)? Is it different from the submodule listed in E3SM/.gitsubmodules?

E3SM-Project/Icepack.git njeffery/fixes-to-bgc

* Do the changes in icepack_aerosol.F90 change the answers for ‘old’ aerosol runs? e.g. is subroutine update_snow_bgc called when tr_aero=T?

None of these changes should impact the old aerosols

* icepack_parameters.F90:  Is grid_o_t used in icepack?  I had removed it from the parameters list because it wasn't used.

This was originally for zsalinity. I believe it can be removed.

* icepack_therm_itd.F90: The first initialization of vicen_init (originally =0, now =vicen(:)) in lateral_melt could be removed completely, since it’s set later.  Or initialize both vicen_init and vsnon_init here and remove the later ones.

* icedrv_init_column.F90: There’s a new cpp, NEWINITZBGC.  Should that be here?  I don't think it's set anywhere in the icepack driver.

FYI @apcraig

@proteanplanet
Copy link
Contributor

Hello @njeffery. Looking through the array of testing being undertaken, I might best help by running B-cases in E3SM. I've never run BGC in a B-case before, but the fastest realistic test platform would be the standard V3 resolution configuration with BGC. Do you know if this is set up? I will await Tony's commits and Elizabeth's D-case checks before I start testing, but make preparations for this B-case testing in the meantime.

@njeffery
Copy link
Contributor Author

njeffery commented Jun 3, 2024

@proteanplanet : I'd like to see a fully coupled bgc run, but I'm not sure that's the best comparison. Kat's been running a pre-Industrial bgc run with master, and it just crashed at year 39 with a MarBL error. Although the MarBL port has been well tested in GCASEs, until now, it hasn't been tested in full coupling. So, we should try a short run. Maybe 20 years?

@apcraig
Copy link
Contributor

apcraig commented Jun 3, 2024

@njeffery, has any testing been done with "debug" build flags. I'm running into several divide by zeros and other things are part of my testing of standalone Icepack. I will try to implement code fixes and then create a PR to your Icepack branch. Then we can discuss whether some of the fixes are due to incorrect setup of a case or other things.

Yes. I've compiled with debug. I'll test again was I've added some of Elizabeth's corrections.

Derecho has been down the last two days for emergency maintenance. Hoping it's back up again today, so I can continue to test. I've run into both divide by zeros and array bounds issues in the standalone testing. I don't know if something about one (or all) of the four columns we test is unusual. Also, I'm testing by a bgc and an skl bgc case. You are running with the debug flags with the bgc on? Are we still supporting skl?

@apcraig
Copy link
Contributor

apcraig commented Jun 3, 2024

@eclare108213 and @apcraig : The corresponding icepack branch is E3SM-Project/Icepack.git njeffery/fixes-to-bgc

@njeffery, Are you working in the E3SM-Project repo or your repo? Just want to make sure I'm using the latest code and want to make sure I PR any suggested changes to the right place. It would be helpful to move forward on a specific branch in a specific repo and stay there. I don't care where that is. Thanks.

@njeffery
Copy link
Contributor Author

njeffery commented Jun 3, 2024

E3SM-Project

@proteanplanet
Copy link
Contributor

proteanplanet commented Jun 3, 2024

@proteanplanet : I'd like to see a fully coupled bgc run, but I'm not sure that's the best comparison. Kat's been running a pre-Industrial bgc run with master, and it just crashed at year 39 with a MarBL error. Although the MarBL port has been well tested in GCASEs, until now, it hasn't been tested in full coupling. So, we should try a short run. Maybe 20 years?

OK. Let me digest this and think about what test we would run in a B-case to demonstrate there are no problems in the full icepack. We need a 100 year simulation for PRs of this kind to be accepted.

@njeffery
Copy link
Contributor Author

njeffery commented Jun 3, 2024

I'd like to try a 100 year zaerosol fully coupled test. I did a long one shortly before Dec when trying to get dust/BC in v3. It doesn't use MarBL so shouldn't crash. It won't replace a bgc test, but works as an additional test.

@njeffery
Copy link
Contributor Author

njeffery commented Jun 3, 2024

@njeffery, has any testing been done with "debug" build flags. I'm running into several divide by zeros and other things are part of my testing of standalone Icepack. I will try to implement code fixes and then create a PR to your Icepack branch. Then we can discuss whether some of the fixes are due to incorrect setup of a case or other things.

Yes. I've compiled with debug. I'll test again was I've added some of Elizabeth's corrections.

Derecho has been down the last two days for emergency maintenance. Hoping it's back up again today, so I can continue to test. I've run into both divide by zeros and array bounds issues in the standalone testing. I don't know if something about one (or all) of the four columns we test is unusual. Also, I'm testing by a bgc and an skl bgc case. You are running with the debug flags with the bgc on? Are we still supporting skl?

I haven't tested skl and don't have plans to test it in E3SM. To my knowledge, it's never been tested in MPAS.

@apcraig
Copy link
Contributor

apcraig commented Jun 3, 2024

@njeffery, has any testing been done with "debug" build flags. I'm running into several divide by zeros and other things are part of my testing of standalone Icepack. I will try to implement code fixes and then create a PR to your Icepack branch. Then we can discuss whether some of the fixes are due to incorrect setup of a case or other things.

Yes. I've compiled with debug. I'll test again was I've added some of Elizabeth's corrections.

Derecho has been down the last two days for emergency maintenance. Hoping it's back up again today, so I can continue to test. I've run into both divide by zeros and array bounds issues in the standalone testing. I don't know if something about one (or all) of the four columns we test is unusual. Also, I'm testing by a bgc and an skl bgc case. You are running with the debug flags with the bgc on? Are we still supporting skl?

I haven't tested skl and don't have plans to test it in E3SM. To my knowledge, it's never been tested in MPAS.

Do we expect skl to work in general and will we continue to support it in the Consortium? If we are, then it's important that it work robustly. For the Consortium, we need to make sure we're clear what bgc configurations are supported and to test all those configurations. What E3SM uses in production is a related by somewhat separate issue.

@apcraig
Copy link
Contributor

apcraig commented Jun 4, 2024

I have created a PR with some changes to Icepack for the Icepack standalone bgc runs. There are still problems that I'm working on but it's at least a start. E3SM-Project/Icepack#37

I have only tested this version of the Icepack model. I will next

  • review all changes relative to the Consortium main
  • test relative to the Consortium main Icepack
  • test in CICE

I am still getting a bgc skl array bounds error in this version of the model with build debug flags on that I need to better understand and may need some help with.

@apcraig
Copy link
Contributor

apcraig commented Jun 4, 2024

There is some new code in icepack_biogeochemistry (see below). This is where the skl debug tests are hitting an array bounds error in this term,

trcrn(bio_index(mm) + k-1,n).

Any thoughts about how this should be fixed?

forrtl: severe (408): fort: (2): Subscript #1 of the array TRCRN has value 39 which is greater than the upper bound of 38
Image              PC                Routine            Line        Source             
icepack            0000000000972446  icepack_zbgc_mp_i         945  icepack_zbgc.F90
icepack            00000000005641A9  icedrv_step_mp_bi        1442  icedrv_step.F90
+         else
+            do mm = 1, nbtrcr
+               do k  = 1, nblyr+1
+                  if (present(flux_bion)) then
+                     flux_bion(mm,n) = flux_bion(mm,n) + trcrn(bio_index(mm) + k-1,n) *  &
+                        hin_old(n) * zspace(k)/dt * trcrn(nt_fbri,n)
+                  endif
+                  flux_bio(mm) = flux_bio(mm) + trcrn(bio_index(mm) + k-1,n) * &
+                     vicen_init(n) * zspace(k)/dt * trcrn(nt_fbri,n)
+                  trcrn(bio_index(mm) + k-1,n) = c0
+                enddo
+            enddo

@apcraig
Copy link
Contributor

apcraig commented Jun 4, 2024

It looks like cgrid, bgrid, igrid, icgrid, and swgrid are all definied at initialization, identical on each column, and fixed throughout the run. These are computed in icepack then passed back to the driver which then never uses the values, but passes them back and forth into icepack. Could these "grid" variables be initialized via a call to icepack_init_hbrine then just stored in Icepack permanently. Just looking at ways to clean up the bgc interfaces.

-Shortened registry description for ocean bgc concentrations
-Corrected some unit descriptions
-Clean-up in mpas_seaice_icepack

bugfix (BFB without bgc/zaerosols)
@njeffery
Copy link
Contributor Author

@eclare108213 and @apcraig : Two 15-year GMPAS tests completed, one with icepack enabled and the other with column_package. See https://acme-climate.atlassian.net/wiki/spaces/ICE/pages/4405002400/dust.icepack.GMPAS.IcoswISC30E3r5.BGCfinal+and+dust.column.GMPAS.IcoswISC30E3r5.BGCall

The carbon conservation member in the column_package version had some spurious high values which appeared now and again. This was traced to errors in the bgc implementation for step_therm2. The Icepack version no longer has these high values.

The sea ice primary production monthly climatology averaged over the last 5 years of the run looks identical in log plots.

I'll merge @apcraig's updates to icepack and let you know when I've verified BFB.

More cleanup of the mpas-seaice icepack interface.
Updates icepack hash #31ce4221
BFB
@njeffery
Copy link
Contributor Author

@eclare108213 and @apcraig : Merged Tony's changes to Icepack and updated the mpas interface. Also verified that the code is BFB.

@apcraig
Copy link
Contributor

apcraig commented Jun 23, 2024

@njeffery, where are the Icepack commit in the E3SM-Project fork. I don't see them on njeffery/fixes-to-bgc or anywhere else, am I missing something?

@njeffery
Copy link
Contributor Author

@apcraig : forgot to push. should be there now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BGC mpas-seaice non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants