-
Notifications
You must be signed in to change notification settings - Fork 0
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
support C-grid connectivity in deseas #39
base: master
Are you sure you want to change the base?
Conversation
@ezhilsabareesh8 could you try this out with your C-grid workflow please? You'll need to add |
@aekiss I tired deseas with
|
OK thanks @ezhilsabareesh8 - referring to this list the only differences I can see are the Malacca Strait is now open and there are a couple more wet points at the mouth of the Baltic. Is your script on github somewhere I can take a look? |
Thanks @aekiss, compared to OM2, I think the Mediterranean, red and black sea are removed in the current workflow, even when the My script is here |
I meant the only noticeable improvements in using |
do the spurious land points also appear using |
Great! With Are Malacca and Sunda straits open? It's hard to tell. |
Sorry for not looking at this earlier. Happy to see that this seems to work as expected. I'll do a more thorough review tomorrow. |
I'm still getting this with
|
@aekiss I'm afraid it's not a bug, but rather a "feature". The maximum number of diffusion iterations is hard-coded, most likely to avoid an infinite loop in case the code has some issue. I would suggest to increase that number to a much larger value. EDIT: better yet, add this value to the CLI, so that the user can choose this value (and keep the default reasonable). |
Oh, scrap that. I now see that the algorithm seems stuck. Yes, definitely a bug. |
I've pushed a commit to this PR that fixes the issue |
…ix_nonadvective, check_nonadvective; require B grid for fix_nonadvective, check_nonadvective
@micaeljtoliveira sorry for the churn, I think it's ready for review now. I've streamlined the code, only requiring |
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.
@aekiss Overall looks good.
That was quite a nasty little bug...
@@ -72,6 +75,19 @@ type(topography_t) function topography_constructor(filename) result(topog) | |||
call handle_error(nf90_get_att(ncid, depth_id, 'maximum_depth', topog%max_depth), isfatal=.false.) | |||
call handle_error(nf90_get_att(ncid, depth_id, 'nonadvective_cells_removed', topog%nonadvective_cells_removed), isfatal=.false.) | |||
|
|||
if (present(grid_type)) then | |||
topog%grid_type = grid_type ! grid_type arg overrides value in file |
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.
Wouldn't it be better to check if the value given in the command line is consistent with the value on the file? Otherwise, if I understand correctly, it's possible to change the grid type on file when running deseas
, which is a bit weird.
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.
This is an intentional feature, otherwise there's no way to alter the grid type on file (which otherwise defaults to B)
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'll add some words to that effect in the docs.
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 main purpose of grid_type in the file is to indicate to fill_fraction, fix_nonadvective and check_nonadvective what connectivity rules to use when checking whether the number of seas has changed, ie to indicate the rules used previously by deseas when lakes_removed was set to 'yes'.
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.
Ah, I see. Yes, this definitely needs to be documented, both in the docs and in the code.
choke_north = .not. (any(sea(im, j:jp) == land) .and. any(sea(ip, j:jp) == land)) | ||
new_sea = min(minval([sea(im, j), sea(ip, j), sea(i, jm), sea(i, jp)], & | ||
mask=[choke_west, choke_east, choke_south, choke_north]), land) | ||
if ( this%grid_type == 'C' ) then |
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.
For cases like this one, I strongly prefer to use the select case construct instead of an if statement, as it's easier to extend and to catch possible mistakes (e.g., right now, if one would have this%grid_type == 'Z'
, the code would run without any error).
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.
thanks, good idea
Co-authored-by: Micael Oliveira <micael.oliveira@anu.edu.au>
fixes #37