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

V4.5.0 #26

Merged
merged 9 commits into from
Oct 27, 2017
Merged

V4.5.0 #26

merged 9 commits into from
Oct 27, 2017

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Oct 21, 2017

One of the external URLs tests is failing, but it still passes with the older version, and Windows fails across the board for Python <3.5 (that is expected from Unidata/netcdf-c#347, sadly no action was taken on that one 😒)

@gillins do you think that patching Windows is a low hanging fruit?

@dopplershift do you have any idea why that

ncdump -h https://data.nodc.noaa.gov/thredds/dodsC/ioos/sccoos/scripps_pier/scripps_pier-2016.nc

returns

ncdump:
https://data.nodc.noaa.gov/thredds/dodsC/ioos/sccoos/scripps_pier/scripps_pier-2016.nc:
NetCDF: Invalid argument

with v4.5.0?

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@gillins
Copy link
Contributor

gillins commented Oct 23, 2017

@ocefpaf it's a big job I'm afraid. c99 syntax now seems to be used throughout (which vc9 can't handle).... I know you had success in one project compiling C files in C++ mode - this may work here but my attempt seem to cause other problems.

Are there any plans to retire Python 2.7 on Windows at least? I know the next major libgdal will also not compile with vc9.

@ocefpaf
Copy link
Member Author

ocefpaf commented Oct 23, 2017

Are there any plans to retire Python 2.7 on Windows at least?

No plans yet but it seems is time to start preparing for that. I don't think it is worth working around this one.

I know the next major libgdal will also not compile with vc9.

I saw that and I guess it will be the Python 2.7 killer for the Python GIS community. (I actually welcomed it :-)

@dopplershift
Copy link
Member

@WardF @DennisHeimbigner can you take a look at that failing URL above?

@DennisHeimbigner
Copy link

Ok, the problem is this.
In .dds, we see

Byte temperature_flagPrimary[time = 94466];

meaning its type is unsigned 8-bit.
In .das, we see these attributes for temperature_flagPrimary

Int16 _FillValue -1;
String _Unsigned "true";
This produces a technical mismatch since the type of the var is not same as the type of _FillValue.
This complicated because DAP2 does not have a signed byte type, only unsigned.
Not sure what is intended or how to fix.
Is there any chance to send me the very original file from the server: scripps_pier-2016.nc ?

@DennisHeimbigner
Copy link

Ok, I got what I believe is the underlying scripps_pier-2016.nc
file. The type of temperature_flagPrimary is ubyte (unsigned byte).
But there is no _FillValue specified. So, I must assume that the .nc ->dap2
code in thredds is adding the (incorrect) _FillValue and the (unnecessary)_IsUnsigned attributes.

@dopplershift
Copy link
Member

@DennisHeimbigner That link works fine on 4.4.1.1--is there any reason to think it working was a bug in the C library?

@DennisHeimbigner
Copy link

DennisHeimbigner commented Oct 23, 2017

The addition of _FillValue and _Unsigned is in thredds 4.x but not in 5.x.
This may be a problem down the road.
I can, I think, fix this in two ways.

  1. Hack the dap2 translation in the c-library to handle the _FillValue: -1
  2. Fix the code in thredds to properly output (in the DAS)
    Byte_FillValue 255;;
    I am guessing that 1 is preferable for you because it is unlikely that you
    can get the server upgraded.

@dopplershift
Copy link
Member

dopplershift commented Oct 23, 2017

1 is preferable because netCDF-c worked just fine on this data in 4.4.1.1--let's not lose sight of the fact that 4.5.0 breaks on a URL that worked fine in the last version, and so far I haven't heard a justification for that behavior. Therefore, I assume that means this is a bug in 4.5.0. 😁

@WardF
Copy link
Contributor

WardF commented Oct 23, 2017

I agree that preserving previous behavior is preferable unless it was a blatant bug. @DennisHeimbigner I feel like we're going to have 4.5.1 out pretty quickly, would this take much to fix on the library side?

@DennisHeimbigner
Copy link

I believe that the 4.4.1.1 behavior is wrong, but since the dap2 translation is already
a complete hack (because of its need to be compatible with the original opendap code)
I have no problem reverting to the previous behavior (assuming no unforeseen side effects).
I believe the real bug is in the server.

DennisHeimbigner added a commit to Unidata/netcdf-c that referenced this pull request Oct 24, 2017
re: conda-forge/libnetcdf-feedstock#26
(Note: note conda-forge, not Unidata).
Revert dap2 code to 4.4.1.1. behavior so that attribute values
are forced into a specific range. Current behavior generates
error if an attribute value is out of range.
@DennisHeimbigner
Copy link

Should be fixed by Unidata/netcdf-c#512

@ocefpaf
Copy link
Member Author

ocefpaf commented Oct 24, 2017

Commit 0eaf9a6 implements Unidata/netcdf-c#512 and it seems to be working 🎉

Thanks for understanding and fixing this @DennisHeimbigner. It would be hard to request tons of data providers out there to update their servers 😬

@ocefpaf ocefpaf merged commit a58fe70 into conda-forge:master Oct 27, 2017
@ocefpaf ocefpaf deleted the v4.5.0 branch October 27, 2017 13:52
mingwandroid pushed a commit to mingwandroid/netcdf-c that referenced this pull request Dec 24, 2017
re: conda-forge/libnetcdf-feedstock#26
(Note: note conda-forge, not Unidata).
Revert dap2 code to 4.4.1.1. behavior so that attribute values
are forced into a specific range. Current behavior generates
error if an attribute value is out of range.
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.

6 participants