-
Notifications
You must be signed in to change notification settings - Fork 19
XIOS: Handle CGField
#951
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
XIOS: Handle CGField
#951
Conversation
5124da0 to
47e77cc
Compare
47e77cc to
5bcc1ce
Compare
1006aa6 to
7ab746e
Compare
5bcc1ce to
4fc926e
Compare
|
[Rebased on top of issue911_xios-dgs] |
4fc926e to
adfb12a
Compare
adfb12a to
536b02b
Compare
timspainNERSC
left a comment
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.
One small, non-essential suggestion: rather than using cice as the name of the CG variable, might I suggest uice, as ice velocity is written by the real model as a CG field, whereas ice concentration never will be.
Thanks @timspainNERSC. I made this change in 8f40681. Given that that variable was being used for diagnostics, I switched to output |
TomMelt
left a comment
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 @jwallwork23 looks good to me. I don't think you need to change anything but I do want to make sure I understand the plan for CG Vectors before we merge this (and my changes #969). I will mark as request changes for now, but after we discuss next meeting I can probs just approve this.
| } else if (dimType == ModelArray::Dimension::XCG) { | ||
| localLength = CGDEGREE * metadata.getLocalExtentX() + 1; | ||
| start = CGDEGREE * metadata.getLocalCornerX(); | ||
| } else if (dimType == ModelArray::Dimension::YCG) { | ||
| localLength = CGDEGREE * metadata.getLocalExtentY() + 1; | ||
| start = CGDEGREE * metadata.getLocalCornerY(); |
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.
FWIW this is also how I am currently determining the CG dims.... but I would like to hear Tim's comments on PR #969 first. I am worried we might end up in a situation where CG dims is set in several different places. I don't think you need to change this, just raising it as an FYI.
core/src/Xios.cpp
Outdated
| throw std::runtime_error("Xios: inconsistent global dimensions for " | ||
| + dimName + ": " + std::to_string(dim.getSize()) + " vs. " | ||
| + std::to_string( | ||
| ModelArray::definedDimensions.at(dimType).globalLength)); | ||
| } | ||
| if (localLength != ModelArray::size(dimType)) { | ||
| throw std::runtime_error( | ||
| "Xios: inconsistent local dimensions for " + dimName); | ||
| if (localLength != ModelArray::definedDimensions.at(dimType).localLength) { | ||
| throw std::runtime_error("Xios: inconsistent local dimensions for " | ||
| + dimName + ": " + std::to_string(localLength) + " vs. " | ||
| + std::to_string( | ||
| ModelArray::definedDimensions.at(dimType).localLength)); | ||
| } | ||
| if (start != ModelArray::definedDimensions.at(dimType).start) { | ||
| throw std::runtime_error("Xios: inconsistent start index for " + dimName); | ||
| throw std::runtime_error("Xios: inconsistent start index for " + dimName | ||
| + ": " + std::to_string(start) + " vs. " | ||
| + std::to_string(ModelArray::definedDimensions.at(dimType).start)); |
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 this alleviates my prev. concern about different sizes for CG dims in the code. However, I might need to understand how this works a bit better. Where will this be called in NextSim? Is it only called once?
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.
My subsequent PR #968 will make it a bit clearer, but it's only called once yeah. In that follow-up PR I have it happening when the Model gets configured, which in turn calls ModelMetadata.setTime.
TomMelt
left a comment
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 @jwallwork23. It's all good for now
# Automation and pruning for the XIOS handler class Towards #850 Merges into #951 ### Task List - [x] Defined the tests that specify a complete and functioning change (*It may help to create a [design specification & test specification](../../../wiki/Specification-Template)*) - [x] Implemented the source code change that satisfies the tests - [x] Documented the feature by providing worked example - [x] Updated the README or other documentation - [x] Completed the pre-Request checklist below --- # Change Description This PR contains refactoring work to (a) automate the XIOS I/O implementation to streamline the user experience and (b) remove member functions that it turns out aren't needed. #### Automation * Separate `affixModelMetadata` into `parseInputFiles`, `setupDomains`, `setupAxes`, and `setupGrids`. Call `parseInputFiles` in the `Xios` constructor and the rest in the `ParaGridIO` constructor. * Automate setting `nCoords` for `VERTEX` in `setupDomains`. * Automate setting `nComponents` for `DG` in `setupDomains`. * Automate setting `nComponents` for `DGSTRESS` in `setupDomains`. * Automate `enableXios` by inlining it at the start of `Model.configureTime`. #### Pruning * Inline `enableXios` in `Model.configureTime` as mentioned above. * Remove `getClientMPISize` and `getClientMPIRank`. * Remove `setAxisValues` and `getAxisValues`. * No need to call `ModelMetadata.setTime` because it already gets called in `Model.configureTime`. * Inline `isInitialized` at the end of the `Xios` constructor, throwing an error if not. * Drop `getCalendarType` since it's hard-coded to `"Gregorian"`. * Drop `getCalendarTimestep` because this is given by `ModelMetadata.stepLength`. --- # Test Description The XIOS handler tests are reworked to account for the automation. This involved some reordering of how things are done. --- # Documentation Impact The XIOS docs page has been updated. --- # Other information I changed the default `DGCOMP` value in `discontinuousgalerkin/ModelArrayDetails.cpp` to be consistent with the default DG degree that gets set in the root `CMakeLists.txt`. --- ### Pre-Request Checklist - [x] The requirements of this pull request are fully captured in an issue or design specification and are linked and summarised in the description of this PR - [x] No new warnings are generated - [x] The documentation has been updated (or an issue has been created to track the corresponding change) - [x] Methods and Tests are commented such that they can be understood without having to obtain additional context - [x] This PR/Issue is labelled as a bug/feature/enhancement/breaking change - [x] This change conforms to the conventions described in the README
XIOS: Handle
CGFieldFixes #911
Merges into #952
Task List
Change Description
Currently the XIOS implementation accounts for
HField,VertexField, andDGField. #952 extends to coverDGSField. This PR extends further to coverCGField.This involved checking the global and local dimensions and start index are set up correctly, as well as the XIOS domain, grid, and field.
Several error messages were also made clearer as part of this change.
Test Description
The XIOS read and write tests are updated to account for the CG field type.
A minor looping issue is also fixed in the value checking - it was missing the final value for
VertexField. As such, the value checking code inXiosRead_testwas reworked to loop over fields first and then dimensions. To make this easier to check, I changed the coordinate field to align with the coordinates of the grid (assuming unit spacing in each direction).Note that the file reading code accounts for halos out-of-the-box! 👼
Documentation Impact
CGFieldis mentioned in the XIOS docs page.Pre-Request Checklist