-
Notifications
You must be signed in to change notification settings - Fork 262
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
Attempt to create NC_STRING var with fill value off causes HDF5 error #727
Comments
I propose to check for this and return an NC_EINVAL if the user tries to turn off fill mode for a string type. |
|
1 Yes, I can produce a HDF5 program. |
Ok. But we need to make sure that there is anote in the documentation |
I don't believe this is a HDF5 bug. If you think about how VLENs have to be stored, it makes sense that a fill value is always required. Unlike with NC_INT or one of the other atomic types, you cannot just calculate how much space an array of 100 NC_STRINGS will take. It's not known. So it makes sense that a placeholder (fill value) is required for any values not explicitly written by the users. So I don't believe this is a bug in HDF5 or will ever change. I have added a note of this to the documentation of nc_dev_var_fill(). I will refrain from writing a HDF5 version of the code to prove this, but I will post on the HDF5 mailing list to confirm I am correct about VLENs always needing fill mode. |
Good point, extcept that NULL would be a perfectly acceptable "fill" value. |
It's not actually NULL. The fill value is a VLEN array of 1 byte containing NULL. Not the same. Not just the NULL is stored. Since it's a VLEN, the length is also stored. |
In memory or in the file? In memory I would be surprised if was not stored |
I have asked on the HDF5 mailing list. However, my understanding is that they treat strings internally as VLENS. They don't take advantage of any special knowledge of the meaning of NULL in strings. We shall see what the HDF5 developers answer. In the meantime, I have changed the docs to explain that fill mode can't be turned off for NC_STRING vars, and changed nc_def_var_fill() so that NC_EINVAL is returned if that is attempted. I will put this branch up as a PR, probably next week. No rush since Ward seems to have plenty of PRs deal with right now. ;-) |
Thank you; I do, plus prepping for AMS. And the holidays and non NetCDF work. Busy but no more so than everybody else I suppose! |
OK, well this is fixed in my branch, along with some other bugs. I think my current branch should probably be in 4.6.0 as well as the ones you have already selected, since it does fix a bunch of problems. But now I am working on the rename problems that Charlie Zender has brought to our attention. These are apparently a problem for the IPCC, so should represent a priority for us. Happy holidays to all indeed. I cannot imagine any gift I would want, other than the opportunity to continue to contribute to atmospheric and climate science, by continuing to develop and test netCDF. ;-) |
What is the 3.6.0 you are referring to? |
I meant 4.6.0, the next release. Scarlet O'Hara: "When I'm wearing a new bonnet, it seems like all the figures I know leave my head." |
Hi Ed, Dennis and I were planning to sit and review the PIO code at the January meeting so that we can wrap our heads around it and make sure it’s something we’re comfortable with before we take ownership of it in the Inidata repository. It also seems like it might be worth getting it’s own release. As it stands, the 4.6 release will probably come first and then we will take a look at this patch. We have suffered in the past for trying to cram too much into a release and we already have the compression filters as the main feature of the current release :). |
I am technically on PTO until January 3rd but I’m going to try to get the 4.6 release out before then so that we can move forward with the review. |
Howdy @WardF I hope you take as much time off as you like, and enjoy the holidays. Getting 4.6.0 out is not something that has to happen in the next few days if that's a hardship. We can all wait until early January. ;-) I also anticipated that you would want to get 4.6.0 out before merging the PIO code. The PIO integration is worthy of it's own release. And I am happy to see all my recent non-PIO bug-fixes, documentation improvements, and warning cleanups make it into a release. The PIO features will give netCDF a really beautiful set of features for supercomputers. With these features we will save the team of the new FV3 model from writing their own version of this functionality (which would be completely proprietary, and not involve netCDF except at the final output stage.) And the FV3 team is just the most important and prominent modeling team in this situation - there are no doubt many others around the world. The FV3 dynamical core is going to become the heart of America's forecasting system. So this is a glittering prize, which will put netCDF at the very center of key operational code for NOAA. NetCDF has the opportunity to provide the HPC features that modelers are desperately calling for, in order to take advantage of massively multi-core machines. Not only will we be enabling a lot of science, through increased efficiency of HPC, we will also be cementing netCDF as the data format and library of choice for HPC for decades to come. The PIO features completely eclipse the HPC features offered by HDF5, pnetcdf, or any other library. You will not, however, be maintaining netCDF/PIO alone. Just as with netCDF-4, I fully understand that the introduction of dramatic new features must be followed with months and years of patient efforts to support, document, and maintain those features. So here I am. What are your other two wishes? ;-) |
BTW I accidently closed this ticket. I have a fix but it's on my branch, not yet merged to master. So I will reopen the ticket. |
(You highlighted the wrong Ward) |
Hi all! Sorry to raise an old topic, but I can't find where this limitation was documented... I don't see it mentioned here: https://www.unidata.ucar.edu/software/netcdf/docs/group__variables.html#gabe75b6aa066e6b10a8cf644fb1c55f83 |
I have been adding some systematic tests for fill values in nc_test/tst_formats.c, in branch ejh_fill_values.
When trying to create an NC_STRING var with fill mode turned off I get this interesting error:
Looks like HDF5 is just not cool with turning off fill mode for NC_STRING.
The text was updated successfully, but these errors were encountered: