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

Extract cubewrite fill value modifications #114

Closed
wants to merge 65 commits into from
Closed

Conversation

blimlim
Copy link
Collaborator

@blimlim blimlim commented Sep 27, 2024

This pr closes #99.

It extract's the cubewrite fill value modifications into a seperate function, adds a custom_fill_val optional argument, and adds unit tests.

Any suggestions or ideas are welcome!

@blimlim
Copy link
Collaborator Author

blimlim commented Sep 27, 2024

There's a slight amount of messiness with the added custom_fill_val argument, and so any ideas or suggestions would be great! Specifically, the final line of fix_fill_value forces the fill value to have the same type as the cube.data, which is required by the netCDF conventions

To prevent e.g. a float custom_fill_val from silently being rounded to an integer, I thought it would be good to raise an error when the types don't match:

if custom_fill_val is not None:
if type(custom_fill_val) == cube.data.dtype:
fill_value = custom_fill_val
else:
msg = (f"custom_fill_val type {type(custom_fill_val)} does not "
f"match cube {cube.name()} data type {cube.data.dtype}.")
raise TypeError(msg)

This will also complain e.g. if a float was going to be rounded down to a float32, however this is stricter than what's applied to the default fill value for floats, which is would get rounded down to a float32 if the the cube's data is float32:

elif cube.data.dtype.kind == 'f':
fill_value = DEFAULT_FILL_VAL_FLOAT
else:
fill_value = default_fillvals[
f"{cube.data.dtype.kind:s}{cube.data.dtype.itemsize:1d}"
]
# Use an array to force the type to match the data type
cube.attributes['missing_value'] = np.array([fill_value], cube.data.dtype)

I don't think it's too important, but mainly wanted to check whether we're happy for the type requirements for the custom_fill_val to be stricter than what's applied to the default values?

@blimlim blimlim marked this pull request as draft September 27, 2024 01:03
@blimlim
Copy link
Collaborator Author

blimlim commented Sep 27, 2024

Ah oops, didn't notice the additional places the fill_value is required. Will fix this now

umpost/um2netcdf.py Outdated Show resolved Hide resolved
@aidanheerdegen aidanheerdegen requested review from aidanheerdegen and removed request for aidanheerdegen October 9, 2024 05:19
umpost/um2netcdf.py Outdated Show resolved Hide resolved
Check that correct default fill values are found based
on a cube's data's type.
"""
fake_cube = DummyCube(12345, "fake_var", attributes={})
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might possible to drop attributes={} when this is de-conflicted with the newer code in main.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Fixed in the new PR #147


assert fill_value == expected_fill_val
# Check new fill value type matches cube's data's type
assert fill_value.dtype == cube_data.dtype
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the type check be skipped now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will keep these ones in, as it would be good to check that the get_default_fill_value() function correctly converts the default fill values to match the cube types.

@blimlim
Copy link
Collaborator Author

blimlim commented Oct 10, 2024

Oh no... I broke the branch. I've set up a clean one in a new PR #147

@blimlim blimlim closed this Oct 10, 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.

Extract cubewrite() fill value to function
2 participants