Skip to content

add_dimension: Define behaviour for spatial dimensions better #496

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

Open
wants to merge 3 commits into
base: draft
Choose a base branch
from

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Jan 24, 2024

Clarified that type spatial always adds a spatial dimension for the z axis.

@m-mohr m-mohr added this to the 2.0.0 milestone Jan 24, 2024
@m-mohr m-mohr marked this pull request as ready for review January 24, 2024 10:46
@clausmichele
Copy link
Member

Why can't x and y be added? A possible use case scenario could consist in:
load_collection over an AOI -> reduce_dimension over x (or y) -> add_dimension x (or y) with an "aggregated" label representing that area.

@soxofaan
Copy link
Member

I agree with @clausmichele , is there a particular reason or background story to add this constraint?

@m-mohr
Copy link
Member Author

m-mohr commented Jan 24, 2024

There's no way to specify the actual axis for the spatial dimensions, so just adding a spatial dimension without axis is ambiguous. The most reasonable (and presumably most common) way out for me was to specify that it's z.

@soxofaan
Copy link
Member

maybe use the name argument as axis identifier in some way?

@m-mohr
Copy link
Member Author

m-mohr commented Jan 25, 2024

That seems a little to implicit for me, what would a user get that specifies lat, lon, long, height, etc. I think defaulting to z is fine. If we ever encounter a usecase for x/y (has anyone encountered one?) we could just add an axis parameter to add_dimension that defaults to z.

@soxofaan
Copy link
Member

what would a user get that specifies lat, lon, long, height, etc.

yeah, but when adding a dimension with name "lat" or "lon" or "long" it also does not make sense to do that with axis "z".

Also what if the cube already has an axis "z"?

What about something like

... If the type is spatial, the axis is the first option that does not exist yet from "x", "y", "z"

or

... If the type is spatial, the axis depends on the dimension name: names "x", "lon", "long" map to axis "x", names "y", "lat" map to "y", and all other options map to "z"

@m-mohr m-mohr force-pushed the add-dimension-z branch from 6ce7b9a to d227ee1 Compare July 16, 2025 19:40
@m-mohr
Copy link
Member Author

m-mohr commented Jul 16, 2025

This makes sense. I've added:

If the type is spatial, the axis is the first option that does not exist yet from x, y, and z. This refers to the axis of the dimension, not the name of the dimension.

The axis depends on the type (according to the cube:dimensions axis property). This makes it irrelevant whether it's named x, lat, lon, y or whatever.

Anyone available for a review? @soxofaan @clausmichele

@m-mohr m-mohr changed the title add_dimension: Type spatial always is always for z axis add_dimension: Define behaviour for spatial dimensions better Jul 16, 2025
@clausmichele
Copy link
Member

@m-mohr what if I ask to add a spatial dimension but x,y,z already exist? Return DimensionExists exception?

@m-mohr
Copy link
Member Author

m-mohr commented Jul 17, 2025

Would it make sense to allow multiple z-dimensions? Not sure :-)

@soxofaan
Copy link
Member

Would it make sense to allow multiple z-dimensions?

Stacking multiple "other" or "band" dimensions is something imaginable I think: e.g. one dimension with labels "NDVI" and "EVI", and another dimension with "mean", "min", "max", "stddev", which would be orthogonal to that.

But spatial dimensions are tied to an axis (x, y or z), and I don't think it make sense, mathematically, to have for example multiple x axes

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

Apart from also adding a note on some DimensionExists error, I think this is already fine

@m-mohr
Copy link
Member Author

m-mohr commented Jul 22, 2025

For x and y, I agree. I'm just unsure about the z dimension. Could we repesent e.g. altitude and pressure at the same time in a dataucbe, which afaik in climate are both related to z?! I'm not really into these climate/atmosphere details and how they handle this.

@soxofaan
Copy link
Member

"altitude" sounds like a spatial dimension along a spatial axis with a spatial resolution (meter or kilometer).

But I would think "pressure" is a dimension of type "other" with a non-spatial unit (Pascal or bar or whatever is most common in practice)

@m-mohr
Copy link
Member Author

m-mohr commented Jul 23, 2025

I think I tend to agree, I just don't want us to block any usecases from Climate or similar domains.

@clausmichele
Copy link
Member

I think I tend to agree, I just don't want us to block any usecases from Climate or similar domains.

@pierocampa maybe could help here, from our recent discussion he seemed to be fine using other type of dimension, at the end it doesn't make any difference in the result

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants