-
Notifications
You must be signed in to change notification settings - Fork 12
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
Implement and finish MPAS dynamical core initialization #267
Implement and finish MPAS dynamical core initialization #267
Conversation
Relative humidity (`relhum`) is a diagnostic variable. It should not be included in MPAS input stream.
More elaborate error checking. Also use a more descriptive name for number of constituents.
* Finish MPAS dynamical core initialization. * Support for deferring the definition of constituents until run-time. * Accessor functions/subroutines for querying: * Local mesh dimensions. * Constituent names and indexes. * Mapping between constituent indexes and MPAS scalar indexes.
It is now possible to construct an arbitrary list of variables to input/output data to/from MPAS dynamical core. This functionality may be overengineered, but it supports what CAM-SIMA needs.
There are two kinds of variables in MPAS: ordinary variables and variable arrays. The old implementation would always skip the latter entirely when encountering them during input. The new implementation correctly distinguishes between the two, and checks whether each variable is eligible to be input.
The instance of MPAS dynamical core resides in `dyn_comp`. `dyn_grid` depends on `dyn_comp`, but not vice versa.
c1e146a
to
0c564c0
Compare
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.
Exciting to see that we can now fully initialize the MPAS dycore in CAM-SIMA, good work!
I do have some change requests though. Of course please let me know if you have any questions or concerns with any of my comments or suggestions. Thanks!
Maintain consistent style with others.
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 to me now! I did have some final cleanup requests, but it doesn't need a re-review (from me at least). Thanks!
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 haven't yet had a chance to try different types of ICs, so I'll likely make a second pass with some further comments.
type(file_desc_t), pointer, intent(in) :: pio_file | ||
|
||
character(*), parameter :: subname = 'dyn_mpas_subdriver::dyn_mpas_init_phase3' | ||
character(strkind) :: mesh_filename | ||
integer :: mesh_format | ||
integer, pointer :: num_scalars => null() | ||
type(mpas_pool_type), pointer :: mpas_pool => null() |
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.
Rather than initialize these here (adding the implicit SAVE
attribute), is there a reason to not just nullify these either at the beginning of this routine or just before they are (hopefully) set? Would doing this remove the need to explicitly nullify these at the end of the subroutine?
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 pushed a731e97 to avoid explicit initialization of pointers.
Thanks for catching this. I don't know why I thought the rule doesn't apply to pointers.
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.
It's possible that I'm wrong, but `5.1 Type declaration statements" of the 2003 standard states:
33 If initialization is =>null-init, object-name shall be a pointer, and its initial association status is disas-
34 sociated.
35 The presence of initialization implies that object-name is saved, except for an object-name in a named
36 common block or an object-name with the PARAMETER attribute. The implied SAVE attribute may
37 be reaffirmed by explicit use of the SAVE attribute in the type declaration statement, by inclusion of
38 the object-name in a SAVE statement (5.2.12), or by the appearance of a SAVE statement without a
39 saved-entity -list in the same scoping unit.
Same logic, but the statement is now more compact.
The statement is now clearer on its intent. Other similar instances in the code already follow this style.
According to "8.4 Initialization", it states "Explicit initialization of a variable that is not in a common block implies the SAVE attribute". This also applies to pointer variables like here. Avoid the implied SAVE attribute by removing explicit initialization. Uphold the principle of least astonishment.
Easier to pinpoint where error occurs.
Zero or negative values are not allowed. Others are just fine.
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.
Apologies for taking so long to complete this review! This all seems good to me.
This PR implements the
dyn_init
subroutine, which is responsible for initializing MPAS dynamical core by one of the following methods:This should be the last step of MPAS dynamical core initialization. After this PR, MPAS dynamical core is ready for time integration.
Implementation notes:
Test steps:
Observe log entries similar to the following in
atm.log.<job-id>.<date>-<time>
: