Skip to content
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

Save GDT20 - Polar Stereographic - take 2 #405

Merged
merged 39 commits into from
Apr 19, 2024
Merged

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Apr 9, 2024

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2024

CLA assistant check
All committers have signed the CLA.

@trexfeathers
Copy link
Contributor Author

@pp-mo I'm looking at this comment - #122 (comment):

I'm thinking that at least one of NorthPolar/SouthPolar will currently fail a save-then-load roundtrip check.
That's a good reason for adding one before merging this.

Could you offer some guidance on this? There is a new integration test but I suspect that's not what you meant.

@trexfeathers trexfeathers marked this pull request as draft April 10, 2024 16:45
@trexfeathers trexfeathers self-assigned this Apr 11, 2024
@pp-mo
Copy link
Member

pp-mo commented Apr 11, 2024

@pp-mo I'm looking at this comment - #122 (comment):

I'm thinking that at least one of NorthPolar/SouthPolar will currently fail a save-then-load roundtrip check.
That's a good reason for adding one before merging this.

Could you offer some guidance on this? There is a new integration test but I suspect that's not what you meant.

I think what this meant is that we should also test with Iris iris.coord_systems.PolarStereographic coordinates systems -- i.e. as well as iris.coord_systems.Stereographic.

In CF terms, a "polar_stereographic" grid mapping must have latitude_of_projection_origin = +90 or -90. It's effectively just a special case of "stereographic", but I guess the polar case was put in first so they needed to keep it.
Iris completely follows CF in this, in having a separate PolarStereographic coordinate system type, presumably so that we can sensibly distinguish "polar" ones, but the PolarStereographic class actually inherits from the Stereographic class, and only adds a distinct type and a modified constructor -- no different behaviour.

So, in terms of iris-grib, I think my old comment meant that data with an Iris PolarStereographic coordinate system (either north or south) should be handled by iris-grib, being treated as a generic 'Stereographic' case. But it's saying that that "would have failed", at some point, for some reason.

So it's probably still worth doing a roundtrip cube-grib-cube test on both North and South cases, and in each case when presented as either a PolarStereographic or Stereographic Iris coordinate system.
Though obviously when loading back, you will always get a PolarStereographic Iris coordinate system (since AFAICT GRIB does not seem to provide a more general case).

@pp-mo
Copy link
Member

pp-mo commented Apr 11, 2024

when loading back, you will always get a PolarStereographic Iris coordinate system

Actually no, to my surprise.
we do it like this
So, we always construct a "generic" Stereographic coord-system. I don't know if there is a particular reason for that.

iris_grib/_save_rules.py Outdated Show resolved Hide resolved
iris_grib/_save_rules.py Show resolved Hide resolved
iris_grib/_save_rules.py Show resolved Hide resolved
@trexfeathers trexfeathers marked this pull request as ready for review April 15, 2024 16:55
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions, but looks very comprehensive to me. Practically good to go.

iris_grib/_save_rules.py Outdated Show resolved Hide resolved
iris_grib/_save_rules.py Outdated Show resolved Hide resolved
iris_grib/_save_rules.py Outdated Show resolved Hide resolved
iris_grib/_save_rules.py Outdated Show resolved Hide resolved
@pp-mo pp-mo merged commit b0f4584 into SciTools:main Apr 19, 2024
10 checks passed
@trexfeathers
Copy link
Contributor Author

Thanks @pp-mo 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants