-
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
Error accessing GOES-16 data over opendap #1614
Comments
Ping @Unidata/thredds . |
Pinging @DennisHeimbigner and I will investigate as well as soon as I’m not in the road. Thanks! |
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.nc works for me using netCDF library version 4.7.3 from conda-forge on Windows. One issue I see is that variable Funny (or not) enough, I can ncdump the same dataset via opendap on thredds-test without issue...probably because when looking through thredds-test, 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 |
The underlying .nc file has this: This odd since the file is netcdf-4, so the type should be ushort |
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. |
Those files violate the rules for fill values. So we can either |
According to the GOES-R Product Definition and User’s Guide (PUG)
|
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 ( |
@DennisHeimbigner How does it violate the rule for fill value? 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. |
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 ***@***.***>, 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.
|
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. |
The type of the variable appears as short, but the _Unsigned |
I have found (so far) the following issues: |
The problem@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.
|
I think the backward compatibility issue arises because |
FYI: the transformations are as follows.
This last point is important. The netcdf library has to produce netcdf-3, So, I see (at least) two problems:
|
Please do not ignore the _Unsigned attribute, which is part of the CDM |
It is type short.
This says once you start reading in values, be careful. But they are still 16 bits.
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
Dangerous (more below), but makes sense in theory to map it like this.
Yes. Above I say "dangerous", because it's not clear which attributes should actually be converted. There are no no clear conventions about that.
If the DAP server didn't try to interpret
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.
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. |
Again, we need to decide if the _Unsigned attribute is intended to have So, on to a hack for the netcdf-c library. As Sean notes, we will also need to fix the server-side code |
Ok, I guess we need to make a decision here.
Note:
All of the conversions assume C language conversion are used. Am I missing anything?
|
I 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. |
I was under the impression, when creating netCDF files (using the C library), a mismatch between variable type and In this particular case, the problem was caused by the the TDS OPeNDAP server choosing to interpret |
@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. |
It depends on where the conversion occurs. If it turns out |
@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:
Here'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:
notice anything different? 😉 |
Ok, I stand corrected. |
This seems to still be a problem with netCDF 4.7.4?
|
re: Unidata#1614 NetCDF has a requirement that the type of a _FillValue attribute be the same as the containing variable. However, it is clear that too many servers do not honor this requirement. So, change the default for DAP2 and DAP4 to allow fill mismatch.
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.
The text was updated successfully, but these errors were encountered: