Skip to content

Conversation

gjm174
Copy link
Contributor

@gjm174 gjm174 commented Sep 3, 2024

Added member and time dimension support into the convert_input_to_xarray_dataset method.

@gjm174 gjm174 requested a review from mats-knmi September 3, 2024 13:53
@gjm174 gjm174 self-assigned this Sep 3, 2024
@mats-knmi
Copy link
Contributor

Looks good, I just have a few questions, which I will put in line

"zerovalue": metadata["zerovalue"],
"zr_a": metadata["zr_a"],
"zr_b": metadata["zr_b"],
"xpixelsize": xpixelsize,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to add the xpixelsize and ypixelsize as metadata, since the dataset has an evenly spaced x and y dimension. So I added these variables there. You can retrieve them from the dataset by doing dataset.x.attrs.stepsize and dataset.y.attrs.stepsize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"zr_a",
"zr_b",
"cartesian_unit",
"yorigin",
Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't think the yorigin needs to be kept as attribute any longer (at least not as attribute of the precip var), since at the end of this method we sort the dataset in the x and y dimension so the yorigin will always be lower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@gjm174 gjm174 force-pushed the xarray/upgrade-convertor branch from 92977df to 810f23a Compare September 5, 2024 12:56
@gjm174 gjm174 requested a review from mats-knmi September 5, 2024 13:01
"zerovalue",
"zr_a",
"zr_b",
"cartesian_unit",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh actually one more thing: the cartesian unit is also already present in the attributes of the x and y dimension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@gjm174 gjm174 force-pushed the xarray/upgrade-convertor branch from 810f23a to d47bbe3 Compare September 9, 2024 09:15
@gjm174 gjm174 requested a review from mats-knmi September 9, 2024 09:18
@mats-knmi mats-knmi merged commit 095e046 into xarray/main Sep 23, 2024
1 check passed
mats-knmi pushed a commit that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants