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 aerosol code for vertical bgc #15

Conversation

eclare108213
Copy link
Collaborator

  • Short (1 sentence) summary of your PR:
    Merge aerosol related changes from MPAS-SI into Icepack
  • Developer(s):
    @njeffery @eclare108213
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    ENTER INFORMATION HERE
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit for original tracers (zbgc off)
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:
    Replaces vertical BGC aerosol code in Icepack with MPAS-SI version, which requires a new parameter, and updates related shortwave code from the same commit. The original aerosol tracer code is unchanged.

icepack_aerosol.F90

  • includes a bug fix for the original aerosol tracers (tr_aero=T), which was not in MPAS-SI: Old aerosol bug fix (tr_aero). CICE-Consortium/Icepack#330
  • replaces contents of subroutine update_snow_bgc. The new subroutine argument bio_index_o is commented out until the call in ice_algae.F90 is also updated.

icepack_parameters.F90

  • adds hs_ssl_min with a hard-coded value. Does this need to be in namelist?

icepack_shortwave.F90

@eclare108213 eclare108213 added bug Something isn't working enhancement New feature or request labels Nov 11, 2022
@eclare108213 eclare108213 self-assigned this Nov 11, 2022
@apcraig
Copy link
Collaborator

apcraig commented Nov 11, 2022

Has any testing been done? Do you need me to do any testing to help?

@eclare108213
Copy link
Collaborator Author

I ran a single regression test, which passed:
./icepack.setup -m conda --env macos --test smoke --testid aerotest1 -s mpassi --bcmp aerobase
although it didn't actually test aerosols! I plan to run a full base suite but want to finalize treatment of the new parameter first. It needs to be made available for init/query/write, and possibly be added to namelist. @njeffery: Is hs_ssl_min a namelist parameter in MPAS-SI?

In CICE/Icepack, I do not expect there to be any answer changes for standard aerosols, and there will definitely be answer changes for zbgc aerosols (the opposite is true for MPAS-SI). @njeffery and I still need to get the ISPOL- (and maybe NICE-) forced comparison tests set up. At the moment, I do not think the zbgc changes in icepack_aerosol.F90 will run because it's missing a new subroutine argument. More work needs to be done on the zbgc port before we can test this, I think.

I also would like to change the names of the subroutines to indicate when they are used. E.g. change update_aerosol to update_aerosol_traero and update_snow_bgc to update_aerosol_zbgc_snow. Other suggestions welcome, including for the zbgc porting process.

@eclare108213 eclare108213 marked this pull request as draft November 11, 2022 13:29
@njeffery
Copy link

@eclare108213 : in mpas-seaice, hs_ssl_min is defined in ice_colpkg_shared, just like hs_ssl and hi_ssl.

! Increase aerosol in snow surface due to deposition
! and vertical cycling : after update_aerosol
! Aerosol in snow for vertical biogeochemistry with mushy thermodynamics
! Called from icepack_algae.F90 when z_tracers=T (replaces update_aerosol)

subroutine update_snow_bgc (dt, nblyr, &
nslyr, &

Choose a reason for hiding this comment

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

Looks like everything is in update_snow_bgc, so this should not impact tr_aero unless z_tracers is on?

Choose a reason for hiding this comment

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

In fact, just tested it out with tr_aero = .true. and NTRAERO=3. It is bfb.

Copy link

@njeffery njeffery left a comment

Choose a reason for hiding this comment

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

Approved by inspection. These changes are consistent with what's currently in mpassi.

@eclare108213
Copy link
Collaborator Author

This PR is really old (Nov 2022!) so we'll probably need to pull its commits into a new branch for further testing, to avoid conflicts. And we probably don't want to merge it directly into E3SM's icepack fork, so the branch would need to be in icepack main, then PR'd back to E3SM. Maybe @apcraig or @dabail10 can help...

@apcraig
Copy link
Collaborator

apcraig commented Sep 20, 2023

I can help next week. Shouldn't be difficult to create a PR in Icepack in the Consortium, then move it over to E3SM Icepack after testing and merging.

@eclare108213
Copy link
Collaborator Author

replacing this PR with #30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants