Iwd #125
Iwd #125
Conversation
There was a problem hiding this comment.
For the expression N2mean(:,:) = SUM( e3w_n(:,:,:) * rn2(:,:,:) * wmask(:,:,:) , DIM=3 ) / (ht_n(:,:) + 1._wp - tmask(:,:,1) ) * tmask(:,:,1) , I am slightly concerned about the term 1._wp - tmask(:,:,1) in the denominator.
I see that it's there to handle regions where ht_n might be zero (presumably where tmask=0). However, in shallow shelf regions, e.g. depths between 50m and mindepth, where tdiss is important, this will add 1 to the depth where tmask=1 (wet points), therefore adding an error.
Arguably, that error could be balanced to some extent by tuning rn_kappa_tdiss, but there could be a spatial gradient in the error if the bathymetry was shallow and sloping. That might make it a bit messy to describe in the paper, as tdiss differs in form from that used in Jayne and St. Laurent / Simmons et al.
An alternative would be to add an IF loop to only calculate tdiss where ht_n > 0._wp and to define tdiss=0 elsewhere (fine if it's initialised to zero anyway).
Also, the final term which multiplies the fraction by tmask(:,:,1) might not be needed. If the sum over k of wmask(:,:,:) in the numerator has the same zero pattern as tmask(:,:,1), then the latter might be superfluous.
There was a problem hiding this comment.
As far as I can see, the current code in MY_SRC_TIDE for this branch does not use the halos for the variable h2rough, i.e. there is nowhere where the index of h2rough extends past the inner sub-domains. This halo exchange initialisation could have been necessary if h2rough had needed to be differenced in space or averaged onto another C-grid point.
Could it be instead that this attempt to squash the bug is superfluous and that a later commit actually fixed it? i.e. does the model now run fine if the line 217 with the extra CALL lbc_lnk(...) is commented out?
I've edited your comment only @jasontempestholt, to reflect that mpp_max is a global max across procs., in case the user thought that nksr would somehow be set to the same value across each proc. Hope you don't mind me committing directly to the IWD branch, as this is only a comment change.
|
h2rough is used to calculate tdiss which is used across halo boundary, hence h3rough needs to be valid in halos. Andrew C's advice is to not assume this is done on reading the data in. Given this is only one on init seams safer to leave in. |
There was a problem hiding this comment.
@jasontempestholt Is step.F90_debug needed? All I can see if I compare this with step.F90 in master is a load of additional write(67,*) (...) statements and related, which I assume are the extras you mentioned in your email to Andrew and I at 1451 on 30/10. Aren't these only to write out values related to your method of debugging rather than material changes?
|
agreed , not the best place for it here feel free to delete and I'll add to a debug branch |
Fair point about tdiss halos, although it's those that we should probably really care about. If we knew that h2rough would not be used other than in the tdiss calculation, as it stands, then a halo exchange immediately after the DO-loops where tdiss is defined as a function of h2rough and N2mean could be cleaner and safest, as it would then catch uninitialised halos in both of these vars. Not critical to this iteration of the code, but it might be something for later perhaps. |
|
we'll want to leave the halo exchange as is - these are super expensive so need to avoid putting them in the time stepping part if possilbe |
|
Of course! Very good point Jason!
Chris
On 1 Nov 2023, at 14:23, jasontempestholt ***@***.***> wrote:
Caution: This email has originated from outside of the organisation. Do not click links or open attachments unless you have verified the sender and content is safe. Thank you.
we'll want to leave the halo exchange as is - these are super expensive so need to avoid putting them in the time stepping part if possilbe
—
Reply to this email directly, view it on GitHub<#125 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AF32LSM5NQMFGSWRRMLRL5DYCJLOBAVCNFSM6AAAAAA6ZD55HCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBZGA2DOOBQGA>.
You are receiving this because your review was requested.Message ID: ***@***.***>
This email and any attachments are intended solely for the use of the named recipients. If you are not the intended recipient you must not use, disclose, copy or distribute this email or any of its attachments and should notify the sender immediately and delete this email from your system.
The National Oceanography Centre (NOC) has taken every reasonable precaution to minimise risk of this email or any attachments containing viruses or malware but the recipient should carry out its own virus and malware checks before opening the attachments. NOC does not accept any liability for any losses or damages which the recipient may sustain due to presence of any viruses.
Opinions, conclusions or other information in this message and attachments that are not related directly to NOC business are solely those of the author and do not represent the views of NOC.
|
|
Please ignore this last comment/question - it’s nonsense!
Chris
On 1 Nov 2023, at 14:21, chris-O-wilson ***@***.***> wrote:
Caution: This email has originated from outside of the organisation. Do not click links or open attachments unless you have verified the sender and content is safe. Thank you.
@chris-O-wilson commented on this pull request.
…________________________________
On MY_SRC_TIDE/sbctide.F90<#125 (comment)>:
As h2rough has been initialised to zero already on line 71, couldn't the following code
! mask hrough in shllow water
DO ji=2,jpim1
DO jj=2,jpjm1
h2rough(ji,jj) = h2rough(ji,jj) * h2rough(ji,jj) ! read in h, need h^2
IF ( gdepw_0(ji,jj,mbkt(ji,jj)+1) < tdiss_mindepth ) THEN
h2rough(ji,jj) = 0.0
ENDIF
ENDDO
ENDDO
be replaced by
! mask hrough in shllow water
DO ji=2,jpim1
DO jj=2,jpjm1
IF ( gdepw_0(ji,jj,mbkt(ji,jj)+1) >= tdiss_mindepth ) THEN
h2rough(ji,jj) = h2rough(ji,jj) * h2rough(ji,jj) ! read in h, need h^2
ENDIF
ENDDO
ENDDO
as it's simpler and more efficient?
—
Reply to this email directly, view it on GitHub<#125 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AF32LSKK4OQY7RBHW7VVSZ3YCJLHTAVCNFSM6AAAAAA6ZD55HCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBYGMZDOMRTHA>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
This email and any attachments are intended solely for the use of the named recipients. If you are not the intended recipient you must not use, disclose, copy or distribute this email or any of its attachments and should notify the sender immediately and delete this email from your system.
The National Oceanography Centre (NOC) has taken every reasonable precaution to minimise risk of this email or any attachments containing viruses or malware but the recipient should carry out its own virus and malware checks before opening the attachments. NOC does not accept any liability for any losses or damages which the recipient may sustain due to presence of any viruses.
Opinions, conclusions or other information in this message and attachments that are not related directly to NOC business are solely those of the author and do not represent the views of NOC.
|
As agreed with @jasontempestholt. The original commit had only added debug diagnostic output.
Correction to N2 mean calculation and initialization of tdiss