-
Notifications
You must be signed in to change notification settings - Fork 18
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
Select short name of variable defining ModelVerticalGrid #3182
base: release/MAPL-v3
Are you sure you want to change the base?
Select short name of variable defining ModelVerticalGrid #3182
Conversation
…hakrab/vertical-regridding
…hakrab/vertical-regridding
class(ModelVerticalGrid), intent(inout) :: this | ||
character(*), intent(in) :: short_name | ||
character(*), optional, intent(in) :: edge |
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.
You need an "unusable" argument here to separate out the options. Both arguments are the same type (character), so it is dangerously ambiguous which one is intended.
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.
Not sure I understand. Both edge
and center
are optional arguments. Did you mean something like
subroutine add_short_names(this, edge, unusable, center)
class(ModelVerticalGrid), intent(inout) :: this
character(*), optional, intent(in) :: edge
class(KeywordEnforder), optional, intent(in) :: unusable
character(*), optional, intent(in) :: center
if (present(edge)) this%short_name_edge = edge
if (present(center)) this%short_name_center = center
_UNUSED_DUMMY(unusable)
end subroutine add_short_names
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.
No - I mean "unusable" should be even before edge. Force the user to be explicit since there is no obvious ordering of "center" vs "edge" in this context.
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.
To be explicit, consider this call:
call obj%add_short_name('x')
Is it obvious that this was adding "edge"?
Or consider:
call obj%add_short_name('x', 'y')
Which is which? Instead:
call obj%add_short_name(edge='x', center='y')
is unambiguous.
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.
Got it. I had seen the unusable
argument, but was not familiar with its usage. Pushed the change.
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 picked up the technique from ESMF. They use it to enforce the rule that interfaces remain backward compatible. All new arguments must be optional and after the "unusable" argument. But the same technique also allows us to enforce using keywords for ambiguous situations. At one point I was going to rigorously enforce that all MAPL optional arguments be after "unusable" as a stylistic statement that makes optional arguments more obviously optional. But am no longer so dogmatic. Becomes a pain to put "unusable" in interfaces that just have optional "rc".
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.
A couple of minor changes requested. See inline comments.
Plus, renamed add_short_names -> add_short_name
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.
Looks good.
Types of change(s)
Checklist
make tests
)Description
standard_name
,units
andnum_levels
. The methodadd_short_name
is used to specify variable short names corresponding to edge and center vertical staggerings, which is selected at runtime using the methodget_short_name
.ComponentSpecParse::parse_geometry_spec
to use the new interface.Test_ModelVerticalGrid.pf
to use the new interface.standard_name
instead ofshort_name
for Model VerticalGrids.MAPL_BaseMod::MAPL_GridGet
after all.Related Issue