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

Bugfix kOBL #182

Merged
merged 2 commits into from
Sep 27, 2022
Merged

Bugfix kOBL #182

merged 2 commits into from
Sep 27, 2022

Conversation

TomasTorsvik
Copy link
Contributor

@milicak @matsbn
Hi,
the assignment of kOBL looks strange to me (see #179). The code change suggested here is according to code description in CVMix, assuming that kOBL should represent the index of the cell center above the OBL depth (this is what we want to use in iHAMOCC). If this was not your intention, we should perhaps define a new index variable according to kt below, that can be imported in iHAMOCC.

DESCRIPTION:
Computes the index of the level and interface above OBL_depth. The index is
stored as a real number, and the integer index can be solved for in the
following way:\
\verb|kt| = index of cell center above OBL_depth = \verb|nint(kOBL_depth)-1|
\verb|kw| = index of interface above OBL_depth = \verb|floor(kOBL_depth)|

@matsbn
Copy link
Contributor

matsbn commented Sep 26, 2022

I think kOBL as used in mod_difest.F is supposed to be the index of the interface (not cell center) above the OBLdepth. Then line 1164 should be kOBL = int(hOBL(i,j)), which seems consistent with the previous implicit truncation when converting from real to integer (better to do the conversion explicitly).

Then in the argument of CVMix_coeffs_conv, I wonder if kOBL is correct to use. This is similar as in MOM6, but the routine cvmix_coeffs_conv_wrap in cvmix_convection.F90 seems to use a different index (similar to nint(hOBL(i,j))+1). I hope @milicak can comment on this.

Would it be an idea to pass hOBL(:,:) to iHAMOCC? Then iHAMOCC can decide on the most appropriate definition (nint(hOBL(i,j))-1 : cell center above OBLdepth; int(hOBL(i,j)) : interface above OBLdepth).

@milicak
Copy link
Collaborator

milicak commented Sep 27, 2022

I agree with @matsbn -1 doesn't make sense to me. we shoudl change it as kOBL = int(hOBL(i,j)).
However, what he suggested is also perfect solution, iHAMOCC can decide what to do with kOBL rather than changing the BLOM code with -1.

@TomasTorsvik
Copy link
Contributor Author

Hi @matsbn , @matsbn
thanks for your comments. I think for the physics you may want to use floor instead of nint, to make sure you always get the interface above hOBL. Since I have this PR already open I have suggested an implementation here (see latest update).

I still need to pass hOBL(i,j) as a field to iHAMOCC, but I can make this change in a separate PR.

@TomasTorsvik
Copy link
Contributor Author

Sorry, int and floor should behave the same way in this case, but nint could have a different output.

@matsbn
Copy link
Contributor

matsbn commented Sep 27, 2022

I would prefer kOBL = int(hOBL(i,j)) over kOBL = floor(hOBL(i,j)). They should behave identical for the positive real values in question here, but kOBL = int(hOBL(i,j)) is consistent with the CVMix comment on how to obtain the interface index (also, although hardly important, int might be slightly more efficient compared to floor).

@TomasTorsvik
Copy link
Contributor Author

I would prefer kOBL = int(hOBL(i,j)) over kOBL = floor(hOBL(i,j)). They should behave identical for the positive real values in question here, but kOBL = int(hOBL(i,j)) is consistent with the CVMix comment on how to obtain the interface index (also, although hardly important, int might be slightly more efficient compared to floor).

Done

Copy link
Contributor

@matsbn matsbn left a comment

Choose a reason for hiding this comment

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

Looks good!

@TomasTorsvik TomasTorsvik merged commit 83d9237 into NorESMhub:master Sep 27, 2022
@TomasTorsvik TomasTorsvik deleted the bugfix_kOBL branch September 27, 2022 11:11
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.

3 participants