-
Notifications
You must be signed in to change notification settings - Fork 300
Heuristically determine if saving a NetCDF attribute as a string or unicode. #2158
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
Conversation
|
Please discuss this as part of #2133, rather than forking the conversation in this PR. |
looking good to me :) |
|
Have updated this to behave consistently for all cases of attribute creation. |
|
@QuLogic in #2133 you requested that the test change be aimed at 1.10.x My view is that this change is more suitable for Iris, explicitly controlling behaviour, rather than putting an 'ignore difference' pattern into our testing strategy. Either of these would be slightly challenging to label a 'bugfix' but I feel more strongly about this one. whilst we are trying to preserve behaviour through changes in netcdf4-python, the result could be that people who already use a more up to date version of netcdf4-python than we test would see this as a behaviour change. as such I'm not comfortable with pushing this to 1.10.x as a bug fix please may you consider this and share with us your views on why it is so desirable to address this issue for iris 1.10? thank you |
|
@pelson thank you |
…hould be a byte string or unicode.
|
I now wish to make it clear that this is my preferred approach for addressing #2133 (comment). The behaviour change that this is addressing covers an untested netCDF4-python, and maintains the longstanding behaviour of Iris. With this change, Iris may make the following statement about saving of NetCDF attributes: "if you provide a string that cannot be encoded as ASCII, we will save it as an NC_STRING, otherwise we will save it as NC_CHAR in all cases". |
marqh
left a comment
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.
ready to merge, from my p.o.v.
|
No tests were added or deleted in this PR simply because this is a "continuation of behaviour" change - essentially all functionality should continue as was originally intended at the time of writing the original functionality. @marqh - if you are ready to merge, please merge. 👍 |
An alternative approach to #2133.