support setting/inquiring per variable fill mode for nc3 files #383

wkliao · 2017-03-24T00:29:15Z

This pull request is related to issue #379.
The current netCDF limits the use of APIs nc_def_var_fill and nc_inq_var_fill to NetCDF-4 files. This request eliminates such limit in order to support per-variable mode for the classic netCDF files. I developed this pull request based on branch ghpull-375 to reuse subroutine NC3_inq_default_fill_value.

Below is the summary of changes.

add a new member, no_fill, to NC_var struct, so a variable can have its own fill mode.
move def_var_fill outside of USE_NETCDF4 ifdef bracket in NC_Dispatch struct. Similar changes to dispatcher objects in libsrc, libsrc4, libsrcp, and libdap2.
move nc_inq_var_fill outside of USE_NETCDF4 ifdef bracket in libdispatch/dvarinq.c
add two new arguments, no_fill and fill_valuep, to the end of NC3_inq_var().
When creating a new attribute with name _FillValue, the variable's fill mode is turned on
When deleting attribute _FillValue, the variable fill mode is turned off
When calling nc_set_fill, fill mode is turned on/off for all variables already defined. Any variables defined successively will also use the same mode.
When calling nc_def_var_fill to turn off fill mode, the variable's attribute _FillValue is deleted, if defined.
During nc_enddef, filling is done only for those variable with the fill mode turned on.

WardF · 2017-03-24T02:47:30Z

I'm not certain we want to delete _FillValue if/when a variables fill mode is turned off. If fill mode is turned off and the _FillValue is removed, there is no way to easily determine what, if any, of the existing data in the variable was created by a fill value and what was 'real' data. That is the only thing that immediately occurs to me off the top of my head; I'll take a look at this tomorrow but I'll want to knock out gphull-375 first in terms of merging. @DennisHeimbigner any thoughts on this?

DennisHeimbigner · 2017-03-24T03:04:00Z

I agree. Once turned on, _FillValue should stay defined for exactly your reason. I understand
the other point of view because of fill is turned off thru the API, then we no longer can
guarantee that the fillvalue won't appear by accident. Perhaps we should consider this proposal:

once fill is turned on, it cannot be turned off.
add an option to nccopy to turn off fill for a specified variable.

… on or off a variable's fill mode

wkliao · 2017-03-24T06:48:52Z

Somehow I mistakenly thought putting/deleting attribute _FillValue automatically turns on/off a variable's fill mode. I just pushed a fix.

wkliao · 2017-03-24T20:52:19Z

FYI
After commit @ac5d8e5 above, test program tst_vars2.c failed.
The cause is related to issue #384

WardF · 2017-03-29T21:49:49Z

Holding off on evaluating this until our internal meeting to review ghpull-375 next week.

…1 and CDF-2 files

needs to be reviewed by maintainers

It turns out that the chunksize used in the ncio-related code must be a multiple of eight in size. Both memio.c and mmapio.c were potentially violating this constraint. See also pr Unidata#400

This reverts commit 09ba25c.

…ease notes.

Fix NC_DISKLESS returns garbage data for certain files

…github.com/rouault/netcdf-c into v3.5.0-release-branch

The problem was that for opendap, it is possible to use keywords as identifiers when there is no ambiguity. However, the DAP2 parser lost the case of the identifier used the lower case version. Fix is to use the actual text of the symbol when it is used as an identifier. Also added a test case for this (kwcase.*). Additionally cleaned up some misc. dap2 testing problems. 1. ncdap_test/tst_ncdap3.sh was using an empty test set. restored the testing of datasets. 2. as a consequence of Unidata#1, some tests needed to be updated with minor tweeks. 3. fix dapmerge to handle multiple DODS_EXTRAS attributes. 4. modify buildattribute to suppress nul characters and terminate the name at the first nul. 5. clean up various test scripts to remove residual, unused references to obsolete netcdf-4 translation. 6. export e.g. NCDUMP from test_common.in so that non-top-level shell scripts can access it.

DennisHeimbigner · 2017-07-01T19:16:15Z

Please do not do this. It makes our job much harder when you do these kinds
of shotgun updates.

wkliao · 2017-07-01T19:32:03Z

I simply merged the master branch to this pull request branch.

DennisHeimbigner · 2017-07-01T20:22:50Z

I am confused. At the top of this pr, it says:
wkliao wants to merge 107 commits into Unidata:ghpull-375 from wkliao:nc3-per-var-fill
ghpull-375 has already been closed. Why are you trying to merge into it?

wkliao · 2017-07-01T23:55:20Z

Note this 383 pull request was created based on ghpull-375 (see my first post).
Github must have kept the original message, even after 375 was merged to master

WardF · 2017-07-06T19:11:12Z

Reviewed w/ @DennisHeimbigner, will go into 4.5.1 after the 4.5.0 release.

WardF · 2017-08-16T16:49:33Z

Still on our radar but trying to get 4.5.0 out the door. Dealing with an issue now related to downstream tools handling _FillValue, see #458 for more information.

edhartnett · 2017-11-03T14:28:49Z

I hope we can get this merged soon.

Firstly, it will be a very useful improvement (I too have felt the lack of this function for classic files - it's a pain).

Secondly, there are a lot of code changes in this PR, and meanwhile I am making a lot of code changes to these files for PIO.

WardF · 2017-11-03T16:53:46Z

It will be a bit; working on some other issues/PR's that predate these ones, and having to switch gears to non-netCDF work. But will return to these ASAP.

edhartnett · 2017-12-20T14:18:43Z

There are an awful lot of extra changes in this PR that have nothing to do with setting the fill value in nc3 files.

What's up with that?

I really want these fill value changes, but I really wonder what is going on with all the other changes. Are we reverting a bunch of code? Some of the changes look quite scary, like changing size_t to off_t. What's going on with that?

@WardF there are loads of cmake build file changes in this PR. Is this expected?

Shall I try and isolate the fill value changes from @wkliao, based on a current version of master, and only including changes related to fill value? Fill values have become a sticking point for me in several tests, so it would be nice if these features could be added, but not if they are going to break stuff.

wkliao · 2017-12-21T03:01:11Z

All my changes to support setting per-variable fill mode in this PR have been extracted to #731.
In addition, I added a test program nc_test/tst_def_var_fill.c

edhartnett · 2017-12-21T12:16:14Z

OK, so should this PR be closed without merging now?

wkliao · 2017-12-21T16:37:32Z

In my opinion, it can be safely closed without merging.

a clean commit for #383

support setting/inquiring per variable fill mode for nc3 files

02d51ca

putting or deleting attribute _FillValue does not automatically turns…

89b6ec2

… on or off a variable's fill mode

Wei-keng Liao added 2 commits March 24, 2017 13:50

fix minor errors in Makefile.am

d23f224

check error code NC_ELATEFILL

ac5d8e5

WardF added this to the 4.4.3 milestone Mar 24, 2017

WardF added status/under review type/bug type/enhancement labels Mar 24, 2017

WardF requested review from DennisHeimbigner and WardF March 24, 2017 21:40

Wei-keng Liao added 2 commits March 25, 2017 14:31

NC_EGLOBAL is returned when trying to set global _FillValue

a5aa720

NC_EGLOBAL is returned when using NC_GLOBAL in nc_inq_var_fill

7a3e935

Wei-keng Liao added 2 commits April 3, 2017 22:07

enable nc_open to check the number of large variables allowed in CDF-…

68a5783

…1 and CDF-2 files

rename macro ERR to ERR_CHK, as ERR is already defined in err_macros.h

1af2643

wkliao mentioned this pull request Apr 4, 2017

patch for issue 384 - NC_ELATEFILL error for NetCDF-4 files #387

Merged

thehesiod and others added 10 commits April 27, 2017 22:26

fix nc_open_mem

674d212

fix typo

90609f6

Merge branch 'master' into nc_open_mem_fix

23c14bd

fix strange garbage results

09ba25c

needs to be reviewed by maintainers

re: issue Unidata#401

6dc6cf6

It turns out that the chunksize used in the ncio-related code must be a multiple of eight in size. Both memio.c and mmapio.c were potentially violating this constraint. See also pr Unidata#400

Add testcase

5a0c7ab

typo

455f3b9

Revert "fix strange garbage results"

a8c8143

This reverts commit 09ba25c.

Corrected a build issue on windows for tst_diskless5, and updated rel…

ce20f3b

…ease notes.

Merge pull request Unidata#403 from Unidata/memio.dmh

e39c26d

Fix NC_DISKLESS returns garbage data for certain files

WardF and others added 8 commits June 26, 2017 11:03

Merge branch 'fix_undefined_left_shift_in_ncx_get_size_t' of https://…

8ee484b

…github.com/rouault/netcdf-c into v3.5.0-release-branch

Forgot to add test files to github

0ae2232

Testing something with the pure h5 tests.

ed4dfb4

Merge remote-tracking branch 'origin/issue310.dmh' into gh434

d73c501

Updated release notes.

fd2c575

merge the latest master branch

228b0c4

patch for dap4 support

db1dcab

czender mentioned this pull request Aug 15, 2017

_FillValue attribute creation error for NC_GLOBAL with netCDF 4.5.x? #458

Closed

wkliao mentioned this pull request Oct 15, 2017

nc_inq_var_fill does not work NETCDF3 (64BIT or CLASSIC) #499

Open

wkliao added a commit to wkliao/netcdf-c that referenced this pull request Dec 21, 2017

a clean commit for Unidata#383

0f4a85b

wkliao mentioned this pull request Dec 21, 2017

a clean commit for #383 #731

Merged

wkliao closed this Dec 21, 2017

WardF added a commit that referenced this pull request Feb 2, 2018

Merge pull request #731 from wkliao/nc3-per-var-fill-v2

8c6defe

a clean commit for #383

wkliao mentioned this pull request Feb 8, 2018

Issue with _FillValue not being written for NetCDF3 files #843

Closed

9 tasks

wkliao mentioned this pull request Aug 16, 2018

change all variables fill mode when nc_set_fill is called #1114

Closed

wkliao deleted the nc3-per-var-fill branch September 15, 2018 16:26

jswhit mentioned this pull request Apr 9, 2019

variable masked_array differences between v1.4.1 and v1.4.2 Unidata/netcdf4-python#908

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support setting/inquiring per variable fill mode for nc3 files #383

support setting/inquiring per variable fill mode for nc3 files #383

wkliao commented Mar 24, 2017

WardF commented Mar 24, 2017

DennisHeimbigner commented Mar 24, 2017

wkliao commented Mar 24, 2017

wkliao commented Mar 24, 2017

WardF commented Mar 29, 2017

DennisHeimbigner commented Jul 1, 2017

wkliao commented Jul 1, 2017

DennisHeimbigner commented Jul 1, 2017

wkliao commented Jul 1, 2017

WardF commented Jul 6, 2017

WardF commented Aug 16, 2017

edhartnett commented Nov 3, 2017

WardF commented Nov 3, 2017

edhartnett commented Dec 20, 2017

wkliao commented Dec 21, 2017

edhartnett commented Dec 21, 2017

wkliao commented Dec 21, 2017

support setting/inquiring per variable fill mode for nc3 files #383

support setting/inquiring per variable fill mode for nc3 files #383

Conversation

wkliao commented Mar 24, 2017

WardF commented Mar 24, 2017

DennisHeimbigner commented Mar 24, 2017

wkliao commented Mar 24, 2017

wkliao commented Mar 24, 2017

WardF commented Mar 29, 2017

DennisHeimbigner commented Jul 1, 2017

wkliao commented Jul 1, 2017

DennisHeimbigner commented Jul 1, 2017

wkliao commented Jul 1, 2017

WardF commented Jul 6, 2017

WardF commented Aug 16, 2017

edhartnett commented Nov 3, 2017

WardF commented Nov 3, 2017

edhartnett commented Dec 20, 2017

wkliao commented Dec 21, 2017

edhartnett commented Dec 21, 2017

wkliao commented Dec 21, 2017