-
Notifications
You must be signed in to change notification settings - Fork 237
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
Re write obcs tides #752
Re write obcs tides #752
Conversation
also enable to add tidal velocity to Tangential Flow
To also test the addition of tangential component of the tidal velocity, import the updated set-up from @antnguyen PR MITgcm#73 (branch "obcs_tides_tangential") including updated ref output from the same PR.
Allow to provide tidal Ampl & Phase bin files with just the number of tidal-comp that are used (instead of the full tidal-comp size "OBCS_tideCompSize") by calling directly pkg/msdio section-read S/R.
Two comments:
|
@menemenlis Any chancee you will find a little bit of time to review this PR ? Thanks. |
A quick q: generally we either use amp & phase or split into sin and cos components , e.g., y = Acos(2pi/T t) + Bsin(2pi/T t) = sqrt(A^2+B^2) cos (2pi/T t - tan^{-1}(B/A)) . Am I correct you're switching from the latter (old, am, ph) to the former (cos and sin)? Also, in the "old" way of am/ph, it was only for the "normal" component, e.g., if eastern boundary, we get u = normal. What you're redoing now, not only to use the sin and cos form (thus getting away from am, ph), but that there'll be an additional sin + cos for the tangential component? Thus the way you've coded now, with cos+sin for u and same for v, naturally one will be "normal" and one will be tangential, right? Thus bypassing the mod code of PR#73 where the "t" am and ph were added? |
pkg/obcs/obcs_add_tides.F
Outdated
& maskW(iB,j,k,bi,bj) * OBEam(j,td,bi,bj) * | ||
& COS( 2.D0 * PI * (myTime-OBEph(j,td,bi,bj)) / | ||
& tidalPeriod(td) ) | ||
& maskW(iB,j,k,bi,bj) * OBE_uTideCs(j,td,bi,bj) * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way the new variables are created, it's a bit confusing to understand this line: i'm used to understanding the form am*cos(2pi/T (t - phase)) , which is what the old code was. Now, it takes the same form, but with variables OBE_uTideCs (<-- to do with cosine projection onto u-component?) replacing "am" and OBE_uTideSn (<-- to do with sine projection onto u-component?) replace "ph" . Are the new variables identical in definition? or are they just place holders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lines you are pointed to are within "#ifdef OLD_OBCS_TIDES" (lines 63 - 124) and I don't disagree with you. This is the reason I added this comment in Aug 22, 2023:
Two comments:
I would like to remove completely the CPP option OLD_OBCS_TIDES that I added in this branch once we are happy with the new implementation. So we don't have to document this new CPP option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you only read this part of the code, then these lines appear to make no sense. But the fields are in "dual-use" and if OLD_OBCS_TIDES
is defined, they have a different meaning: then OB?_uTideCs = OB?am
and OBN_uTideSn = OB?ph
and this corresponds to what one would expect.
I think the only way to avoid the admittedly confusing code is to keep variables with the old names just for the OLD_OBCS_TIDES
code. Doing so only makes sense (too me), if we plan on keeping the OLD_OBCS_TIDES
code for bitwise reproducibility. But as I understand, this is not the plan.
The way I understand this PR, one can reproduce the old results by making sure that OB?_uTideCs = OB?_vTideCs =OB?am
and OBN_uTideSn = OBN_vTideSn = OB?ph
by specifying the appropriate input fields (subject to roundoff error), so the OLD_OBCS_TIDES
code is not necessary and can be removed. This would also remove the confusion.
Neither the old code nor the new code is yet properly documented anywhere. I'd vote for removing OLD_OBCS_TIDES
code and document the new code properly (and maybe how to recover old results with the new code).
@antnguyen13 Thanks for reviewing this and regarding you questions:
You are right, and this match this PR description:
And to reiterate, we continue to specify (in input files) the amplitude and phase (no changes here) but we store (in common block) the pair product: Amplitude x cos,sin [2piPhase/Period] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the changes look good to me. I have these comments:
- At first sight, it is complicated to relate the new code to the old one, but that's maybe not a big problem.
- Related to the previous comment: With this PR the results of
seaice_obcs.tides
are very different. This may be intentional, but would it be helpful to create results that would reproduce results of the old code? I tried by using the oldOB?am/ph.seaice_obcs
and adjustingdata.obc
like this:
OBCS_tidalPeriod = 44714.16,43200.,45569.88,43081.92,86164.2,92949.48,86637.24,96726.24,1180295.64,2380716.,
OBS_vTidamFile='OBSam.seaice_obcs',
OBS_vTidphFile='OBSph.seaice_obcs',
OBN_vTidamFile='OBNam.seaice_obcs',
OBN_vTidphFile='OBNph.seaice_obcs',
OBE_uTidamFile='OBEam.seaice_obcs',
OBE_uTidphFile='OBEph.seaice_obcs',
OBW_uTidamFile='OBWam.seaice_obcs',
OBW_uTidphFile='OBWph.seaice_obcs',
useOBCStides = .TRUE.,
and got nearly identical results (13 matching digits on my MacBook, when it was 14 with upstream/master).
- If we can illustrate or describe somewhere how one can reproduce the old results with the new code without the cpp-flag
OLD_OBCS_TIDES
, then we can certainly get rid off this cpp-flag and the corresponding code (and should).
& OBN_uTidAmFile, OBS_uTidAmFile, OBE_uTidAmFile, OBW_uTidAmFile, | ||
& OBN_vTidAmFile, OBS_vTidAmFile, OBE_vTidAmFile, OBW_vTidAmFile, | ||
& OBN_uTidPhFile, OBS_uTidPhFile, OBE_uTidPhFile, OBW_uTidPhFile, | ||
& OBN_vTidPhFile, OBS_vTidPhFile, OBE_vTidPhFile, OBW_vTidPhFile, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason for the truncated naming? Why not call them OBN_uTideAmpltFile
and OBN_uTidePhaseFile
or OBN_uTidalAmpltFile
and OBN_uTidalPhaseFile
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other reference to field name within OB file-name parameter are short or very short (e.g., "u" instead of "uVel", "S" instead of "Salt"), so I would prefer to keep these ones short too (as were the previous names, with "am" and "ph" for Amplitude and Phase).
@mjlosch Thanks for the review and the coments. Will think about file-names and other things but regarding this:
The "new"
|
I think it is enough to document somewhere (maybe in
by new code and
This would provide some sort of recipe for recovering old results in different contexts, if required. |
@mjlosch I like your suggestion, I will do one or the other (REAME or data.obcs). Also will remove completely |
And provide some instruction on how to update previous data.obcs to work with this updated code.
@mjlosch I removed all the And @menemenlis we are still waiting for you approval (you seems to agreee with issue #617, and this PR is mostly implementing that). |
I'll push small modifications to the |
@mjlosch the README.md now looks even better ! thanks. |
It looks good to me, ready for merge, but a few minor comments for your consideration:
Note: naming of tidal input files (OB*File) have been changed and augmented to accomodate tangential flows in PR #752
C Note that during initialization, the Cs arrays contain Amplitude |
@menemenlis Thanks for the review. I made little adjustment to the README.md file in
is locally in
so it's not obvious to me that we need to repeat/re-phrase it there. So at the end, would prefer to leave it as it is now. |
I've jsut updated this PR description (including suggested addition to tag-index) so, from my side, this PR is ready to be merged (as I understood, @menemenlis approval extends to @antnguyen13 approval) and will merge it in the coming days unless someone wants to add something. |
What changes does this PR introduce?
Implement suggestions from issue #617, renaming OB tidal forcing variables and improving
efficiency of tidal component addition. Also allows to add tides to OB tangential velocity.
What is the current behaviour?
What is the new behaviour
Address all 3 points above.
Also enable to specify the tangential tidal velocity at the OB (An other implementation of PR #73).
Does this PR introduce a breaking change?
Names of Tidal forcing parameters have changed. A check and stop has been added to
obcs_readparams.F
if old-names were found indata.obcs
.Other information:
obcs_init_variables.F
toobcs_init_fixed.F
since the tidal components amplitude and phase are kept constant during one simulation.a temporary CPP option "OLD_OBCS_TIDES" was allowing to recover the original results (but got removed).
seaice_obcs.tides
from PR Yoshi Nakayama code for tangential component of tides #73 has been moved here with minor renaming and smaller binary files (since only 4 components are used).Suggested addition to
tag-index
o pkg/obcs (tides):
with both Cos & Sin of phase times Amplitude stored in common block ;
with non-zero tidal period) ;
phase of each OB barotropic tidal velocity component input files) ;
components (only 4) but including few tangential components.