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

icepack_therm_vertical: add new local variables delt, deltq instead of using worka, workb #375

Open
phil-blain opened this issue Oct 20, 2021 · 1 comment

Comments

@phil-blain
Copy link
Member

While comparing CICE6 with CICE4 in the context of my project, I noticed that the variables worka, workb in subroutine icepack_step_therm1 should probably be renamed to delt, deltq (or at least new variables created with these names).

They are declared here:

real (kind=dbl_kind) :: &
rnslyr , & ! 1 / nslyr
worka, workb ! temporary variables

And sent to icepack_atm_boundary here:

call icepack_atm_boundary('ice', &
Tsfc(n), potT, &
uatm, vatm, &
wind, zlvl, &
Qa, rhoa, &
strairxn, strairyn, &
Trefn, Qrefn, &
worka, workb, &

where they are received as the delt and delq arguments:

subroutine icepack_atm_boundary(sfctype, &
Tsf, potT, &
uatm, vatm, &
wind, zlvl, &
Qa, rhoa, &
strx, stry, &
Tref, Qref, &
delt, delq, &

I notice writing this that worka and workb are also used in other more recent computations.

Anyway, these names date back to the use of global, temporary work arrays in CICE4 and it would be a good clean up to add new local variables with names delt and delq and pass those to icepack_atm_boundary, I think...

I also noticed the following:

  • delt and deltq are marked intent(inout) in icepack_atm_boundary, atmo_boundary_const and atmo_boundary_layer but are really intent(out); their value is not used before being assigned in those subroutines.
@eclare108213
Copy link
Contributor

Changing the names of local variables is fine with me, if it makes the code easier to follow. Reusing work arrays was originally to minimize memory use but we can hardly claim to be efficient with that anymore.

intent(inout) was used to prevent an extra copy when passing arguments in and out of subroutines, and so was used throughout the code for this small bit of computational efficiency gain. For some reason, intent(out) did an extra copy that (inout) didn't have to do. I'm not sure whether this is still an issue with newer compilers. The tradeoff, of course, is some confusion about whether the variable actually needs to be passed in. I used it as a convenient excuse for gathering initializations into one code block in a higher routine.

@phil-blain phil-blain self-assigned this Dec 15, 2022
phil-blain added a commit to phil-blain/Icepack that referenced this issue Feb 6, 2024
In icepack_therm_vertical::icepack_step_therm1, the local variables
'worka' and 'workb' are sent to icepack_atmo::icepack_atm_boundary as
the 'delt' and 'delq' arguments. This is the sole use of these variables
in icepack_step_therm1.

These names date back to the use of global, temporary work arrays in
CICE4. Even if their values are not used, let's make the code clearer by
renaming the local variables delt and delq.

Since 'worka' is also used later on for snow redistribution computation,
let's keep its declaration.

Closes CICE-Consortium#375
phil-blain added a commit to phil-blain/Icepack that referenced this issue Feb 7, 2024
In icepack_therm_vertical::icepack_step_therm1, the local variables
'worka' and 'workb' are sent to icepack_atmo::icepack_atm_boundary as
the 'delt' and 'delq' arguments. This is the sole use of these variables
in icepack_step_therm1.

These names date back to the use of global, temporary work arrays in
CICE4. Even if their values are not used, let's make the code clearer by
renaming the local variables delt and delq.

Since 'worka' is also used later on for snow redistribution computation,
let's keep its declaration.

Closes CICE-Consortium#375
phil-blain added a commit to phil-blain/Icepack that referenced this issue Feb 12, 2024
In icepack_therm_vertical::icepack_step_therm1, the local variables
'worka' and 'workb' are sent to icepack_atmo::icepack_atm_boundary as
the 'delt' and 'delq' arguments. This is the sole use of these variables
in icepack_step_therm1.

These names date back to the use of global, temporary work arrays in
CICE4. Even if their values are not used, let's make the code clearer by
renaming the local variables delt and delq.

Closes CICE-Consortium#375
phil-blain added a commit to phil-blain/Icepack that referenced this issue Feb 12, 2024
In icepack_therm_vertical::icepack_step_therm1, the local variables
'worka' and 'workb' are sent to icepack_atmo::icepack_atm_boundary as
the 'delt' and 'delq' arguments. This is the sole use of these variables
in icepack_step_therm1.

These names date back to the use of global, temporary work arrays in
CICE4. Even if their values are not used, let's make the code clearer by
renaming the local variables delt and delq.

Closes CICE-Consortium#375
phil-blain added a commit to phil-blain/Icepack that referenced this issue Feb 12, 2024
In icepack_therm_vertical::icepack_step_therm1, the local variables
'worka' and 'workb' are sent to icepack_atmo::icepack_atm_boundary as
the 'delt' and 'delq' arguments. This is the sole use of these variables
in icepack_step_therm1.

These names date back to the use of global, temporary work arrays in
CICE4. Even if their values are not used, let's make the code clearer by
renaming the local variables delt and delq.

Since 'worka' is also used later on for snow redistribution computation,
let's keep its declaration.

Closes CICE-Consortium#375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants