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

Update CICE to run with eclare108213/Icepack branch snicar #100

Merged
merged 6 commits into from
Sep 30, 2022

Conversation

apcraig
Copy link
Owner

@apcraig apcraig commented Sep 12, 2022

- including eclare108213/Icepack#13, Sept 11, 2022
- Passes full CICE test suite on cheyenne with 3 compilers except alt04 changes
  answers for all compilers and all tests.  CICE #fea412a55f was baseline.
- Icepack submodule still points to standard version on main, need to be
  swapped manually to appropriate development version.
@apcraig
Copy link
Owner Author

apcraig commented Sep 12, 2022

To summarize changes

  • get rid of faero_optics which sets modal aerosol table properties. Now set by calling icepack_init_radiation. Get rid of all supporting arrays/settings, optics_file, optics_file_fieldname, kaer_tab, waer_tab, gaer_tab, kaer_bc_tab, waer_bc_tab, gaer_bc_tab, bcenh
  • Modify all versions of CICE_InitMod to remove call to faero_optics and add call to icepack_init_radiation
  • Add snw_ssp_table to namelist
  • Add support for dEdd_snicar_ad namelist option
  • Modify icepack_prep_radiation and icepack_step_radiation calls, remove arguments
  • Add support in Makefile and cice.settings for USE_SNICARHC cpp
  • Add set_env/set_nml .snicar and add snicar test case

Copy link
Collaborator

@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.

This looks fine to me, and nothing here stands out as causing alt04 differences. Must be something in Icepack that's not getting triggered in its few grid cells?

@eclare108213
Copy link
Collaborator

Does the new snicar test run successfully?

@apcraig
Copy link
Owner Author

apcraig commented Sep 12, 2022

The new snicar test does run successfully, but not in the test suite for reasons I understand. I'll fix that now and just make sure we're handling the testing correctly. I haven't looked at any results though for dEdd_snicar_ad.

@apcraig
Copy link
Owner Author

apcraig commented Sep 12, 2022

FYI, github actions fails because it doesn't have the correct Icepack version by default.

@eclare108213
Copy link
Collaborator

Documenting our offline conversation, for the record (edited for brevity).

@apcraig:

After several test runs, it looks like the alt04 case is not bit-for-bit on snicar and main because tr_aero=.true. AND calc_tsfc=.true. If either of those are set to false, the case is bit-for-bit. Other test cases with tr_aero=.true. OR calc_tsfc=.true. pass, just not that combination together.

@eclare108213:

That's odd. Or maybe not:
Setting calc_Tsfc = false basically turns off a significant portion of the thermodynamics calculation, so it isn't a very helpful data point.
tr_aero is known to be buggy, of course, but I think it has more of a role in dEdd than in the thermo, and dEdd is mostly what's changing in this code comparison. If you set tr_aero = false in the shortwave module (and nowhere else, so it's still carried around as a tracer), does it become BFB?

@apcraig:

Yes, turning off tr_aero in icepack_shortwave in icepack makes the answers bit-for-bit when tr_aero is on.

At the first timestep, the only diagnostic that is different is "average albedo",

average albedo = 1.00000000000000000 0.85728257197008262
average albedo = 1.00000000000000000 0.85728257197089264

This may be roundoff, but I think it's probably a little bigger. Also, in this case, I am compiling with the debug flags, so I don't think compiler optimization is playing a role, although it could be. It's more likely a change in the computation of some terms associated with the average albedo calculation, maybe roundoff, maybe more. On the second timestep, other diagnostics diverge.

I agree about your point about calc_Tsfc. I guess what I was trying to figure out is why alt03 passes (which has tr_aero=T) while alt04 fails (which also has tr_aero=T). That difference is at least partly a result of the different setting of calc_Tsfc.

@eclare108213:

I've looked through the code differences, focusing on tr_aero code blocks, and I don't see an obvious non-BFB culprit. Weren't there some precision differences in the data tables that were in the cice bgc forcing module? Those might affect aerosol calculations, especially if the forcing data wasn't originally available to icepack.

The only other possibilities are these:

  • the tabular data was moved from (k,w,g)aer_tab to (k,w,g)aer_3bd in a separate module, and maybe that shift in memory is enough of a change
  • the diabolical calculations of g, w0, tau were fixed
  • I did change some integer-style constants to be explicitly _dbl_kind, but I think that should only affect modal_aero=T cases.

@apcraig
Copy link
Owner Author

apcraig commented Sep 18, 2022

I've got bit for bit now. I modified the current Icepack main to use the fixed g,w0,tau calculation, and I also had to update the CICE ice_forcing_bgc tables to full double precision. Both changed answers and now I'm getting bit-for-bit with the snicar branch for the alt04 test case. I have also verified bit-for-bit for the alt04 test case when debug is turned off. So I think this is all good now. Thanks for looking into this and helping sort it out.

The alt04 case was the only non bit-for-bit results with CICE main and now this seems to be fixed. Verifying again.

@apcraig
Copy link
Owner Author

apcraig commented Sep 21, 2022

One other notes. This adds test for modal_aerosol both with and without bgcz.

But one thing that still isn't working is dEdd_algae. I tried to turn that on again and debug a bit, but couldn't get it to work. That bug seems to be caused by (at least) incorrect logic in setting nlt_chl_sw between init_zbgc and count_tracers. nlt_chl_sw is 0 even though it's computed to be 1 in init_bgcz. That results in an error in ice_diagnostics_bgc referencing an array location incorrectly. So this seems to be a problem in tracer indexing when turning on dEdd_algae. At least, that's the first issue.

@apcraig
Copy link
Owner Author

apcraig commented Sep 30, 2022

I have updated the Icepack submodule to point to E3SM-Project/Icepack:cice-consortium/E3SM-icepack-initial-integration #8aef3f785ceb7b, the "snicar" merge. I am retesting now on cheyenne then will merge. This should be bit-for-bit with https://github.com/CICE-Consortium/CICE/tree/6399af75260479a66675ad2b00ffc24ec46d2e10, will regression test against that version and report back here shortly.

@apcraig
Copy link
Owner Author

apcraig commented Sep 30, 2022

Test results vs CICE Consortium #6399af752 all pass and are bit-for-bit, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#d4b9add2977cb8f4e09e322bfef98a7a0f72a369. Github Actions also passes now with the update to the Icepack version. I am merging now.

@apcraig apcraig merged commit 3ca3a81 into E3SMIcepackDEV Sep 30, 2022
apcraig added a commit that referenced this pull request Oct 5, 2023
* Update CICE to run with eclare108213/Icepack branch snicar (#100)

* Update CICE to run with eclare108213/Icepack branch snicar

- including eclare108213/Icepack#13, Sept 11, 2022
- Passes full CICE test suite on cheyenne with 3 compilers except alt04 changes
  answers for all compilers and all tests.  CICE #fea412a55f was baseline.
- Icepack submodule still points to standard version on main, need to be
  swapped manually to appropriate development version.

* Remove faero_optics

* update ciceexe string to account for USE_SNICARHC CPP

* Update documentation

* Update test suite to add modal testing

* Point Icepack submodule to cice-consortium/E3SM-icepack-initial-integration

Update to snicar branch merge, #8aef3f785ce

* Add E3SM namelists for CICE. (#101)

* New e3sm and e3smbgc namelist options

* Update E3SM test options

* Add a simple e3sm test suite

* atmbndy is not actually different

* Additional changes

* add Tliquidus_max namelist parameter to CICE

* Add Tf argument to icepack interfaces

* Add constant option for tfrz_option

* Fix some diagnostic prints and add to additional drivers

* Update messages and change option in alt01

* Update implementation for latest version of Icepack

- Update tfrz_option, add _old options for backwards bit-for-bit
- Fix unittests
- Add hi_min to namelist and tests

* Update Icepack

* Update to E3SM-Project/Icepack/cice-consortium/E3SM-icepack-initial-integration including Icepack1.3.3 release, Dec 15, 2022.

* Update Icepack to E3SM-Project/Icepack #87db73ba6d93747a9, current head of cice-consortium/E3SM-icepack-initial-integration Feb 3, 2023

* Update boxchan1e and boxchan1n tests to tfrz_option = 'mushy_old' to recover Consortium main results

Update Icepack to the latest hash on E3SM-Project Icepack cice-consortium/E3SM-icepack-initial-integration, #96f2fc707fc743d7

Prior commit was a merge from CICE Consortium Main, #d466031001cf447bcd64220c842dcd2707f61e9, Sept 29, 2023

* remove icepack

* update icepack

---------

Co-authored-by: David A. Bailey <[email protected]>
Co-authored-by: Elizabeth Hunke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants