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

Deprecate zsalinity #439

Closed
apcraig opened this issue Jun 19, 2023 · 8 comments · Fixed by #448
Closed

Deprecate zsalinity #439

apcraig opened this issue Jun 19, 2023 · 8 comments · Fixed by #448
Assignees
Labels

Comments

@apcraig
Copy link
Contributor

apcraig commented Jun 19, 2023

I'm in the process of deprecating zsalinity. It looks like the "UNDEPRECATE" implementation done earlier just made it impossible to run with zsalinity, but there is lots of code that actually has to still be removed. Is this correct? Should I try to do that or is this better done by someone with a better understanding of zsalinity?

@apcraig apcraig self-assigned this Jun 19, 2023
@eclare108213
Copy link
Contributor

Is the code that still needs to be removed in the zsalinity module, or elsewhere?

@apcraig
Copy link
Contributor Author

apcraig commented Jun 20, 2023

There is stuff other places. The zsalinity module will be deleted. But for instance, see below. The icepack parameters is fine, I guess we want to leave the setting there, but abort if it's turned on for backwards compatibility. I'm happy to start stripping out the solve_zsal blocks of code. Is solve_zsal just for zsalinity or are there other uses of that flag? Are there some other variables we can key off of to verify the zsalinity is completely removed?

icepack.depzsal/columnphysics>grep -i solve_zsal *.F90
icepack_itd.F90:      use icepack_parameters, only: solve_zsal, skl_bgc, z_tracers
icepack_itd.F90:            if (solve_zsal) then
icepack_itd.F90:            if (solve_zsal) then
icepack_itd.F90:            endif ! solve_zsal
icepack_parameters.F90:         solve_zsal   = .false. ,&! if true, update salinity profile from solve_S_dt
icepack_parameters.F90:         modal_aero_in, skl_bgc_in, solve_zsal_in, grid_o_in, l_sk_in, &
icepack_parameters.F90:         solve_zsal_in          ! if true, update salinity profile from solve_S_dt
icepack_parameters.F90:      if (present(solve_zsal_in)) then
icepack_parameters.F90:         if (solve_zsal_in) then
icepack_parameters.F90:         modal_aero_out, skl_bgc_out, solve_zsal_out, grid_o_out, l_sk_out, &
icepack_parameters.F90:         solve_zsal_out          ! if true, update salinity profile from solve_S_dt
icepack_parameters.F90:      if (present(solve_zsal_out)        ) solve_zsal_out   = solve_zsal
icepack_parameters.F90:        write(iounit,*) "  solve_zsal = ", solve_zsal
icepack_therm_bl99.F90:      use icepack_parameters, only: conduct, calc_Tsfc, solve_zsal
icepack_therm_bl99.F90:      if (solve_zsal) sw_dtemp = p1  ! lower tolerance with dynamic salinity
icepack_therm_itd.F90:      use icepack_parameters, only: z_tracers, solve_zsal, hfrazilmin
icepack_therm_itd.F90:         if (solve_zsal .or. z_tracers) &
icepack_therm_itd.F90:      if (solve_zsal) fzsal = fzsal - rhosi*vi0new/dt*p001*sss*salt_loss
icepack_therm_itd.F90:                     if (.not. solve_zsal) &
icepack_therm_itd.F90:               if (.NOT. solve_zsal)&
icepack_zbgc.F90:      use icepack_parameters, only: scale_bgc, ktherm, skl_bgc, solve_zsal
icepack_zbgc.F90:                                       solve_zsal,   bgrid, &
icepack_zbgc.F90:                                       solve_zsal,      bgrid,  &
icepack_zbgc.F90:            if (solve_zsal .and. vsnon1 .le. c0) then
icepack_zbgc.F90:            endif        ! solve_zsal
icepack_zbgc.F90:      if (solve_zsal) then
icepack_zbgc.F90:                                        solve_zsal, bgrid, &
icepack_zbgc.F90:         solve_zsal
icepack_zbgc.F90:         if (solve_zsal) then
icepack_zbgc.F90:         endif  ! solve_zsal
icepack_zbgc.F90:         if (solve_zsal) then
icepack_zbgc.F90:         endif           ! solve_zsal
icepack_zbgc.F90:            if (solve_zsal .and. k < nblyr + 1) then
icepack_zbgc.F90:            endif                    ! solve_zsal
icepack_zbgc.F90:         if (solve_zsal) then
icepack_zbgc.F90:         endif  ! solve_zsal
icepack_zbgc.F90:            if (scale_bgc .and. solve_zsal) then ! bulk concentration (mmol or mg per m^3)
icepack_zbgc.F90:            if (solve_zsal) trcrn(nt_bgc_S:nt_bgc_S+nblyr-1,n) = c0
icepack_zbgc.F90:               if (solve_zsal)  then
icepack_zbgc.F90:               endif ! solve_zsal

@eclare108213
Copy link
Contributor

Wow, I missed a lot. I think it's fine to remove all of it, and run the various test suites to make sure it doesn't mess up something else. Removing all of it might affect the hbrine tracer - that's the only thing I can think of to keep an eye on. zsalinity was originally developed for zBGC, which we will update and thoroughly test as part of the Icepack-E3SM merge; now the BGC uses the mushy thermo salinity rather than this zsalinity parameterization.

@apcraig
Copy link
Contributor Author

apcraig commented Jun 20, 2023

OK, let me take a crack at removing the code and see what happens. Will keep you posted. I'm pretty sure you intentionally just turned off the zsalinity code during the initial deprecation without removing all the code because it was a bit invasive. That was a reasonable first step that would be easier to do and undo if we decided not to deprecate the feature. But now it does need to happen.

@apcraig
Copy link
Contributor Author

apcraig commented Jul 26, 2023

When we deprecate zsalinity in Icepack, I assume we also want to deprecate zsalinity in the Icepack driver and CICE? In Icepack, I kept the option to set solve_zsal to true, but there is no more solve_zsal code and setting solve_zsal to true will lead to an Icepack abort. I would take the same approach in the Icepack driver and CICE, keep solve_zsal as a namelist, abort if it's set to true, and otherwise remove code we no longer need. Does that sound like the way we want to go? An alternative is just to keep all the solve_zsal code in the Icepack driver and CICE, Icepack will abort if we try to use it. I don't think we want the solve_zsal code laying around in the drivers though. Thoughts?

@eclare108213
Copy link
Contributor

I agree, it's better to remove as much of the zsal code as possible but keep the namelist options so the abort will be clean and provide useful diagnostic information.

@apcraig
Copy link
Contributor Author

apcraig commented Jul 26, 2023

Looks like nt_bgc_S and bgc_S in general is only used with zsalinity. Does that sound right? Is there any reason to keep this tracer index for some future need? Otherwise, I'll remove it, right?

@eclare108213
Copy link
Contributor

Yes, I think that's right.

@apcraig apcraig mentioned this issue Jul 29, 2023
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants