Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cice grid generation #6
Cice grid generation #6
Changes from 6 commits
b53e07d
9a1682f
4a6e8e4
75f4353
01ceaa6
177d807
d1b0cdb
7d6bec5
bf45b0a
e33d9d1
1b5c8e3
f500eb7
04cc749
a93cdbb
871d410
c505109
f0e9cfd
819204c
469ecfc
e1296bc
b518fbd
bc6fadc
cf57e0c
029b399
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we should remove this hardcoding. What's wrong with using the default chunksize?
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 just used these to be consistent with the rest of the om2 cice output. There's a slim chance it helps with mapping reading the file to the right PE's. It also might make it easier to merge with this with the output data.
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.
Ok good. This is done through your PR
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.
Why has this been removed? Will this potentially break existing uses of the code?
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 not used in CICE5 or CICE6. Maybe it was just added to be consistent with all the other grid types in the package? I think because its not used it just adds confusion rather than value.
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.
As above, does this really need to be removed?
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 definitely don't understand the details of CRSs. Is "tripolar_latitude_longitude" really a valid grid mapping? I can't find anything about this in the CF conventions
Is this CRS metadata really valid generally, or are there assumptions about the input grid here? Incorrect CRS info is arguably worse than no CRS info
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 tricky. Our data relies on having 2d arrays of lat/lon (i.e. geolocation arrays) to be used. I haven't found a way to describe it using a proj code or a grid_mapping description.
So the crs here applies to the geolocation arrays. Without the geolocation arrays it doesn't really make sense.
So its the crs_wkt that matters here, thats the information that could (in theory) be used by some piece of analysis software. I just tried to give a descriptive grid_mapping name.
The assumptions are that the input grid is a tripolar input grid in WGS84. The whole thing falls over if that is not true because of the wrapping and folding of the grid. So one day we will need to extend this code for regional grids.
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.
So should the
grid_mapping_name
attribute be removed?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.
ok done
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 agree that it's nice to provide a script to generate a CICE grid from a MOM grid, but I don't think this is the right place to house this code. The grid classes are written to enable conversion between all different types of implemented grids. Scripts to do conversions between specific grids should live elsewhere. These can also be registered as
console-scripts
in theesmgrids
package so they're more readily usable.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.
Ok good. This is done through your PR