dopplershift
commented
4 years ago Ping @Unidata/thredds .
Closed dopplershift closed 3 years ago
dopplershift
commented
4 years ago Ping @Unidata/thredds .
WardF
commented
4 years ago Pinging @DennisHeimbigner and I will investigate as well as soon as I’m not in the road. Thanks!
lesserwhirls
commented
4 years ago Same as Ryan, however:
ncdump -h [fillmismatch]https://thredds.unidata.ucar.edu/thredds/dodsC/satellite/goes/east/grb/ABI/CONUS/Channel13/current/OR_ABI-L1b-RadC-M6C13_G16_s20200302236114_e20200302238499_c20200302238530.ncworks for me using netCDF library version 4.7.3 from conda-forge on Windows.
One issue I see is that variable star_id is UInt16, while its _FillValue is -1.
Funny (or not) enough, I can ncdump the same dataset via opendap on thredds-test without issue...probably because when looking through thredds-test, star_id is Int16. So, this works for me:
ncdump -h https://thredds-test.unidata.ucar.edu/thredds/dodsC/satellite/goes/east/grb/ABI/CONUS/Channel13/current/OR_ABI-L1b-RadC-M6C13_G16_s20200302236114_e20200302238499_c20200302238530.nc
DennisHeimbigner
commented
4 years ago The underlying .nc file has this: short star_id(num_star_looks) ; star_id:_FillValue = -1s ; star_id:long_name = "ABI star catalog identifier associated with observed star" ; star_id:_Unsigned = "true" ; star_id:coordinates = "band_id band_wavelength_star_look t_star_look" ;
This odd since the file is netcdf-4, so the type should be ushort and the _Unsigned attribute should not be there. But this causes the fill mismatch error. Or the type really is short and the _Unsigned is an error.
dopplershift
commented
4 years ago We can talk all we want about whether the attributes make sense. But what matters here is:
So while we could try to reach out to NESDIS to adjust these files, that would be a nicety. That won’t be part of how we solve the problem.
DennisHeimbigner
commented
4 years ago Those files violate the rules for fill values. So we can either change the rules, or require use of [fillmismatch] or something else.
lesserwhirls
commented
4 years ago According to the GOES-R Product Definition and User’s Guide (PUG) Volume 3: Level 1b Products , it is indeed unsigned:
5.0.2 Unsigned Integer Processing The classic model for netCDF (used by the GS) does not support unsigned integers larger than 8 bits. Many of the variables in GOES-R netCDF files are unsigned integers of 16-bit or 32-bit length. The following process is recommended to convert these unsigned integers:
- Retrieve the variable data from the netCDF file.
- For this variable, retrieve the attribute “_Unsigned”.
- If the “_Unsigned” attribute is set to “true” or “True”, then cast the variable data to be unsigned. The steps above must be completed before applying the scale_factor and add_offset values to convert from scaled integer to science units. Also, the valid_range and _FillValue attribute values are to be governed by the “_Unsigned” attribute.
lesserwhirls
commented
4 years ago Those files violate the rules for fill values. So we can either change the rules, or require use of [fillmismatch] or something else.
The netCDF file does not violate the rules though, right? I mean, this is perfectly ok:
short star_id(num_star_looks) ;
star_id:_FillValue = -1s ;
star_id:long_name = "ABI star catalog identifier associated with observed star" ;
star_id:_Unsigned = "true" ;
star_id:coordinates = "band_id band_wavelength_star_look t_star_look" ;The opendap server presentation of the file in TDS 4.6.x is invalid, though. In 4.6.x, it looks like the opendap code tries do the right thing by handling the translation of CDM (short, _Unsigned=true) → DAP2 (UInt16) and expose the data as being UInt16 (DAP), but it does not update the _FillValue attribute (or outright remove it)...it also does not touch the valid_range attribute, either. In 5.0.x, the server simply exposes what is actually in the file without trying to translate.
dopplershift
commented
4 years ago @DennisHeimbigner How does it violate the rule for fill value? _FillValue is a short value, as is the variable. I'm not sure whether it's the right use of _Unsigned, but bluntly: that's not part of the netCDF data model. It's part of "best practices" listed here. Therefore there's no reason that it's use should render netcdf-c unable to work with the file.
I'll also mention again that this USED TO WORK. Therefore we already changed the rules, and in the process broke a real-world workflow. That's a bug on our end.
WardF
commented
4 years ago I am still on the road and have not had a chance to investigate this myself (hello - from a hotel in St. George, Utah), but it sounds like the issue has been diagnosed. This mismatch has cropped up a few times since we added the code to, I believe, enforce an actual rule.
Dennis, you may recall better than I, but I recall the discussion we had around this originally; is it a best practice or was this laid out in the data standard (as I seem to remember?). If the latter, do you have the link? I will see if I can find it when I’m not working from my phone.
Regardless, in the short term we have the [fillmismatch] DAP argument we can use, and we can discuss this at the next netcdf meeting. If it is in fact simply a best practice (I don’t doubt that’s where Ryan is seeing it, but I also recall us having a stronger case for introducing the change to enforce it), we may have to decide if it is worth enforcing and, if not, reverse ourselves in the face of these issues that keep cropping up.
Thanks all,
Ward On Jan 30, 2020, 9:26 PM -0700, Ryan May notifications@github.com, wrote:
@DennisHeimbigner How does it violate the rule for fill value? _FillValue is a short value, as is the variable. I'm not sure whether it's the right use of _Unsigned, but bluntly: that's not part of the netCDF data model. It's part of "best practices" listed here. Therefore there's no reason that it's use should render netcdf-c unable to work with the file. I'll also mention again that this USED TO WORK. Therefore we already changed the rules, and in the process broke a real-world workflow. That's a bug on our end. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.
edwardhartnett
commented
4 years ago I don't understand how this file violates the fill value rules.
However, I believe the fill value checks are a bug and should be removed. As @dopplershift points out, these rules make files and software that used to work, fail.
I think that fill value type checking is a good idea that came 25 years too late. Now netcdf is stuck with all the existing files and software that did not follow the fill value rules.
DennisHeimbigner
commented
4 years ago The type of the variable appears as short, but the _Unsigned attribute means it is converted to ushort. The _FillValue is -1 which is in conflict with ushort.
DennisHeimbigner
commented
4 years ago dopplershift
commented
4 years ago @WardF @DennisHeimbigner When I say we need to fix it, I don't necessarily mean the fix needs to be in netCDF-c. Given that the local file can be read just fine, but it's when going over OPENDAP that it fails, it's completely plausible that the problem is in the TDS OPENDAP server code. Regardless, the fact that we can't use this major data source from our own server and we used to be able to is...not a good look for us.
What I am saying is that any idea that we can just throw up our hands and say that the problem is with the data provider, and/or putting additional burden on our users is a non-starter. Do not pass go. Do not collect $200.
_UnsignedRegarding _Unsigned, here's what's in the netCDF C docs:
(Appendix B)
Note on byte data: It is possible to interpret byte data as either signed (-128 to 127) or unsigned (0 to 255). When reading byte data through an interface that converts it into another numeric type, the default interpretation is signed. There are various attribute conventions for specifying whether bytes represent signed or unsigned data, but no standard convention has been established. The variable attribute “_Unsigned” is reserved for this purpose in future implementations.
(Best Practices for Writing)
NetCDF-3 does not have unsigned integer primitive types.
To be completely safe with unknown readers, widen the data type, or use floating point. You can use the corresponding signed types to store unsigned data only if all client programs know how to interpret this correctly. A new proposed convention is to create a variable attribute _Unsigned = "true" to indicate that integer data should be treated as unsigned.
A conventional way to indicate whether a byte, short, or int variable is meant to be interpreted as unsigned, even for the netCDF-3 classic model that has no external unsigned integer type, is by providing the special variable attribute _Unsigned with value "true". However, most existing data for which packed values are intended to be interpreted as unsigned are stored without this attribute, so readers must be aware of packing assumptions in this case. In the enhanced netCDF-4 data model, packed integers may be declared to be of the appropriate unsigned type.
Those are the only sections of the netCDF docs (grepping the entire docs/ directory in the netcdf-c repo) that mention _Unsigned. The NUG also mentions the same verbiage about _Unsigned under "Best Practices".
In my opinion, none of that text rises to the level of a standard that we have any business ever enforcing. And if it was a standard, it would have text describing, unambiguously, how to encode/interpret other attributes like _FillValue for such variables. Based on the text there currently, I can see no rationale for how you can say that the data provider in this case is doing something incorrect.
The problem is that yes, _FillValue needs to match the variable, which it does because both are signed short. I'm pretty sure this is enforced by netCDF-C. When OPENDAP decides to interpret _Unsigned it makes the variable an unsigned short, but it leaves the _FillValue alone. This is not the data creator's problem, they did everything right. The OPENDAP code should simultaneously be casting the _FillValue if it's going to interpret _Unsigned.
I'm not a huge fan of the behavior in netCDF-C right now to enforce the _FillValue matching on read. I don't know what led to starting its enforcement, but here's what I do know:
_FillValue agreement on write. I don't believe enforcing it on read by default accomplishes anything. In the overwhelming amount of use cases, users reading the data have nothing to do with the data creation; this is true even moreso in the case of remote data on a server. Therefore, they are not in a place to do anything about the "malformed" data. All we're accomplishing is getting in their way, and at best they're going to send us the support ticket.[fillmismatch] is indeed documented in the opendap section of the netcdf-c docs, it's not in the netcdf4-python, or in the xarray docs. This is going to become an increasing support burden.DennisHeimbigner
commented
4 years ago I think the backward compatibility issue arises because the fillvalue match requirement is in the spec, but the code never enforced it until recently. So if we change that behavior we need to change the spec.
DennisHeimbigner
commented
4 years ago FYI: the transformations are as follows.
We start with a netcdf-4 file where
short star_id(num_star_looks) -- not that it is of type short star_id:_FillValue = -1s -- therefor is of type short star_id:_Unsigned = "true"
Looks like this might have been written by some version of netcdf-java because of the _Unsigned attribute; but it was an old version before Caron added true unsigned types.
The DAP code translates this into the DDS form:
UInt16 star_id[num_star_looks = 24] -- the short + _Unsigned => UInt16
The DAP code translates the attributes into this DAS:
star_id { Int16 _FillValue -1; String long_name "ABI star catalog identifier associated with observed star"; String _Unsigned "true"; String coordinates "band_id band_wavelength_star_look t_star_look"; Int32 _ChunkSizes 24; }
Note that the _FillValue type is still short. The server-side DAP code should probably have converted it to UInt16 by using _Unsigned as was done in the DDS.
The netcdf-c library DAP code converts the DDS + DAS (with #fillmismatch):
short star_id(num_star_looks) ; star_id:_FillValue = -1s ; star_id:long_name = "ABI star catalog identifier associated with observed star" ; star_id:_Unsigned = "true" ; star_id:coordinates = "band_id band_wavelength_star_look t_star_look" ; star_id:_ChunkSizes = 24 ;
WARNING: do not be fooled, this last netcdf file is netcdf-3, not netcdf-4. DAP does not know about netcdf-4, so it translates the DDS+DAS to netcdf-3 only.
This last point is important. The netcdf library has to produce netcdf-3, so it has to figure out what to do with the fact that the DDS is giving it an unsigned short. However, also note that the DAS was short (not unsigned short) so it should have been ok if the check was done after star_id was converted to short + _Unsigned on the client side.
So, I see (at least) two problems:
We can fix #2, but probably not #1.
DennisHeimbigner
commented
4 years ago To accomplish what exactly? It's not a bug, because netcdf-c doesn't choke on the exact same CDL when opened locally.
Please do not ignore the _Unsigned attribute, which is part of the CDM representation and has semantic meaning in CDM, and hence must be interpreted by the server-side CDM->DAP2 translation code and the DAP2->NETCDF-3 client-side translation code.
lesserwhirls
commented
4 years ago FYI: the transformations are as follows.
- We start with a netcdf-4 file where short star_id(num_star_looks) -- not that it is of type short
It is type short.
star_id:_FillValue = -1s -- therefor is of type short star_id:_Unsigned = "true"
This says once you start reading in values, be careful. But they are still 16 bits.
Looks like this might have been written by some version of netcdf-java because of the _Unsigned attribute; but it was an old version before Caron added true unsigned types.
_Unsigned was added as an attribute because netCDF-Java was reading in other formats that had unsigned types. Not a problem until you try to write those out to a file, and netCDF-3 was all there was at the time. So the choice was either to widen the type (and inflate the size of the file), or come up with a convention to represent that the data should be interpreted as unsigned. NetCDF-C would have run into the same issue if it had user defined types at the time and only netCDF-3 to write.
Not that any of that matters in this specific case, because these data are produced by CSPP Geo GRB, which is python (so using netCDF-C). Here, the unsigned attribute exists primarily because they are trying to follow the NetCDF CF Metadata Conventions, which does not allow for unsigned types (it has been proposed for the next version). The vague mentions of a proposed convention in the NUG and the lack of getting it formally into the NUG, combined with CFs multiple references to the NUG, lead people down the road of using the _Unsigned attribute.
- The DAP code translates this into the DDS form: UInt16 star_id[num_star_looks = 24] -- the short + _Unsigned => UInt16
Dangerous (more below), but makes sense in theory to map it like this.
- The DAP code translates the attributes into this DAS: star_id { Int16 _FillValue -1; String long_name "ABI star catalog identifier associated with observed star"; String _Unsigned "true"; String coordinates "band_id band_wavelength_star_look t_star_look"; Int32 _ChunkSizes 24; } Note that the _FillValue type is still short. The server-side DAP code should probably have converted it to UInt16 by using _Unsigned as was done in the DDS.
Yes. Above I say "dangerous", because it's not clear which attributes should actually be converted. There are no no clear conventions about that.
- The netcdf-c library DAP code converts the DDS + DAS (with #fillmismatch): short star_id(num_star_looks) ; star_id:_FillValue = -1s ; star_id:long_name = "ABI star catalog identifier associated with observed star" ; star_id:_Unsigned = "true" ; star_id:coordinates = "band_id band_wavelength_star_look t_star_look" ; star_id:_ChunkSizes = 24 ; WARNING: do not be fooled, this last netcdf file is netcdf-3, not netcdf-4. DAP does not know about netcdf-4, so it translates the DDS+DAS to netcdf-3 only.
This last point is important. The netcdf library has to produce netcdf-3, so it has to figure out what to do with the fact that the DDS is giving it an unsigned short. However, also note that the DAS was short (not unsigned short) so it should have been ok if the check was done after star_id was converted to short + _Unsigned on the client side.
If the DAP server didn't try to interpret _Unsigned, the mapping to netCDF Classic would be done before the client sees it, like in TDS 5, yes?
So, I see (at least) two problems:
- server-side when serving up netcdf-4 using old, pre unsigned types, netcdf-java code is inconsistent.
The code in the opendap module is not fully converting, but because of the ambiguity around which attributes should be converted, it's probably best to not try to convert at all on the server side.
- client-side when applying the fillvalue mismatch test needs to be done after the attribute _FIllValue has been properly converted.
We can fix #2, but probably not #1.
We certainly can address the first one (and I believe already is in 5). The issue there is it takes time for the fix to show up in the wild.
DennisHeimbigner
commented
4 years ago Again, we need to decide if the _Unsigned attribute is intended to have semantic meaning, and if so, when is it to be interpreted. I say yes and it is interpreted in the Java code (either client or server). I also believe it should be interpreted by the DAP2 code on the server and client. (we know it is already on the server side).
So, on to a hack for the netcdf-c library. The only soln I can see is to revert to ignoring fill mismatch This means the spec and NUG need to be changed. I suppose that changing the spec requires going thru the original formal process for certifying the spec.
As Sean notes, we will also need to fix the server-side code (DAP2 and probably the code that converts a netcdf-4 file to CDM). But this is going to have no effect in the short to medium term because of the slow adoption of new versions of TDS.
DennisHeimbigner
commented
4 years ago Ok, I guess we need to make a decision here. I propose to partially revert the behavior and remove the requirement that FillValue type == variable type when the type is numeric. I propose to implement this basically as follows:
Note:
All of the conversions assume C language conversion are used.
Am I missing anything?
2. Converting float/double to an integer is two stepI agree that checking should be turned off. @DennisHeimbigner you have outlined a sensible system.
My only question is: how did older versions of the library actually handle an incorrect fill value type? Did it just cast the value to the correct type?
Was the _Unsigned attribute considered with the fill value? I believe not. Yet the GOES data seems to assume that it does. So I am confused about that.
What we don't want to do is change the value that is used as a fill value in the GOES data. Whatever rules are adopted, we have to ensure that the existing GOES data is still readable and the fill values are interpreted correctly.
dopplershift
commented
4 years ago I was under the impression, when creating netCDF files (using the C library), a mismatch between variable type and _FillValue was impossible. As I understand it, this is only an issue with OPeNDAP servers, which don't have this restriction (i.e. it's caused by mapping OPeNDAP to the netcdf data model). That's why the fillmismatch option only exists for opendap urls.
In this particular case, the problem was caused by the the TDS OPeNDAP server choosing to interpret _Unsigned and adjust the variable type, WITHOUT adjusting the _FillValue. So while I think the resolution above is a fine way to reduce errors for netcdf-c users accessing data over OPeNDAP, we still need to do something about THREDDS inconsistently applying _Unsigned. We control this code, so we can and should fix the issue there.
WardF
commented
4 years ago @dopplershift It should be impossible and currently is, but I always shy away from saying it was absolutely always impossible with every version of the C library that has existed or is actively being used.
That is neither here nor there, however; the solution Dennis suggests look good to me as well.
DennisHeimbigner
commented
4 years ago It depends on where the conversion occurs. If it turns out that the converter from some format (netcdf-4, grib, etc) to CDM is causing the problem then I believe any Java code reading thru CDM may encounter this problem. In theory, this should not happen if the CDM code is one that includes unsigned types (when did that change occur?).
dopplershift
commented
4 years ago @DennisHeimbigner I'm pretty sure it's only the TDS OPeNDAP code that's a problem here. Here's the CDL when you go to the cdmremote endpoint:
short star_id(num_star_looks=24);
:_FillValue = -1S; // short
:long_name = "ABI star catalog identifier associated with observed star";
:_Unsigned = "true";
:coordinates = "band_id band_wavelength_star_look t_star_look";
:_ChunkSizes = 24; // intHere's a sample of the ISO XML:
<gco:MemberName>
<gco:aName>
<gco:CharacterString>star_id</gco:CharacterString>
</gco:aName>
<gco:attributeType>
<gco:TypeName>
<gco:aName>
<gco:CharacterString>short</gco:CharacterString>
</gco:aName>
</gco:TypeName>
</gco:attributeType>
</gco:MemberName>and here's the ncml representation as well:
<ncml:variable name="star_id" shape="num_star_looks" type="short">
<ncml:attribute name="_FillValue" type="short" value="-1"/>
<ncml:attribute name="long_name" value="ABI star catalog identifier associated with observed star"/>
<ncml:attribute name="_Unsigned" value="true"/>
<ncml:attribute name="coordinates" value="band_id band_wavelength_star_look t_star_look"/>
<ncml:attribute name="_ChunkSizes" type="int" value="24"/>
</ncml:variable>and here's the DDS from OPeNDAP:
Dataset {
Int16 y[y = 1500];
Int16 x[x = 2500];
Grid {
ARRAY:
UInt16 Rad[y = 1500][x = 2500];
MAPS:
Int16 y[y = 1500];
Int16 x[x = 2500];
} Rad;
Grid {
ARRAY:
Byte DQF[y = 1500][x = 2500];
MAPS:
Int16 y[y = 1500];
Int16 x[x = 2500];
} DQF;
Float64 t;
Float64 time_bounds[number_of_time_bounds = 2];
Int32 goes_imager_projection;
Float32 y_image;
Float32 y_image_bounds[number_of_image_bounds = 2];
Float32 x_image;
Float32 x_image_bounds[number_of_image_bounds = 2];
Float32 nominal_satellite_subpoint_lat;
Float32 nominal_satellite_subpoint_lon;
Float32 nominal_satellite_height;
Float32 geospatial_lat_lon_extent;
Byte yaw_flip_flag;
Byte band_id[band = 1];
Float32 band_wavelength[band = 1];
Float32 esun;
Float32 kappa0;
Float32 planck_fk1;
Float32 planck_fk2;
Float32 planck_bc1;
Float32 planck_bc2;
Int32 valid_pixel_count;
Int32 missing_pixel_count;
Int32 saturated_pixel_count;
Int32 undersaturated_pixel_count;
Int32 focal_plane_temperature_threshold_exceeded_count;
Float32 min_radiance_value_of_valid_pixels;
Float32 max_radiance_value_of_valid_pixels;
Float32 mean_radiance_value_of_valid_pixels;
Float32 std_dev_radiance_value_of_valid_pixels;
Float32 maximum_focal_plane_temperature;
Float32 focal_plane_temperature_threshold_increasing;
Float32 focal_plane_temperature_threshold_decreasing;
Float32 percent_uncorrectable_L0_errors;
Float32 earth_sun_distance_anomaly_in_AU;
Int32 algorithm_dynamic_input_data_container;
Int32 processing_parm_version_container;
Int32 algorithm_product_version_container;
Float64 t_star_look[num_star_looks = 24];
Float32 band_wavelength_star_look[num_star_looks = 24];
UInt16 star_id[num_star_looks = 24];
} satellite/goes/east/grb/ABI/CONUS/Channel13/current/OR_ABI-L1b-RadC-M6C13_G16_s20200372016068_e20200372018453_c20200372018493.nc;notice anything different? :wink:
DennisHeimbigner
commented
4 years ago Ok, I stand corrected. Since DAP2 converts to netcdf-3, my proposal above should still stand except that the lines about conversion to unsigned should be removed.
dopplershift
commented
3 years ago This seems to still be a problem with netCDF 4.7.4?
ncdump -h https://thredds.ucar.edu/thredds/dodsC/satellite/goes/east/grb/ABI/CONUS/Channel08/20210104/OR_ABI-L1b-RadC-M6C08_G16_s20210042321170_e20210042323543_c20210042324042.nc
ncdump: https://thredds.ucar.edu/thredds/dodsC/satellite/goes/east/grb/ABI/CONUS/Channel08/20210104/OR_ABI-L1b-RadC-M6C08_G16_s20210042321170_e20210042323543_c20210042324042.nc: https://thredds.ucar.edu/thredds/dodsC/satellite/goes/east/grb/ABI/CONUS/Channel08/20210104/OR_ABI-L1b-RadC-M6C08_G16_s20210042321170_e20210042323543_c20210042324042.nc: NetCDF: Not a valid data type or _FillValue type mismatch
❯ conda list libnetcdf
# packages in environment at /Users/rmay/miniconda3/envs/py39:
#
# Name Version Build Channel
libnetcdf 4.7.4 nompi_h9d8a93f_107 conda-forge
I've run into another issues hitting opendap from the TDS:
gives
If I download that same file and open locally, it works fine. I'm not sure if this is a recurrence of the issue in #1316 or not. This is happening for me on netCDF-C 4.7.3, installed from conda-forge on macOS 10.15.3. I should note that I'm not having problems with OPeNDAP on any other datasets on thredds.unidata.ucar.edu.