Add support for multiple filters per variable. #1639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
re: Unidata#1584 Support has been added for multiple filters per variable. This affects a number of components in netcdf. The new APIs are documented in NUG/filters.md. The primary changes are: * A set of new functions are provided (see __include/netcdf_filter.h__). - Obtain a list of the filters associated with a variable - Obtain the parameters for a specific filter. * The existing __nc_inq_var_filter__ function now returns info about the first defined filter. * The utilities (ncgen, ncdump, and nccopy) now support an extended format for specifying a sequence of filters. The general form is __<filter>|<filter>..._. * The ncdump **_Filter** attribute now dumps a list of all the filters associated with a variable using the above new format. * Filter specifications can now use a filter name instead of number for filters known to the netcdf library, which in turn is taken from the HDF5 filter registration page. * New errors are defined: NC_EFILTER and NC_ENOFILTER. The latter is returned if an attempt is made to access an unknown filter. * Internally, the dispatch table has been extended to add a function to handle all of the filter functions. * New, filter-related, tests were added to nc_test4. * A new plugin was added to the plugins directory to help with testing. Notes: 1. The shuffle and fletcher32 filters are not part of the multifilter system. Misc. changes: 1. A debug module was added to libhdf5 to help catch error locations.
|
Thanks @DennisHeimbigner, reviewing now! Looks good so far! |
|
These conflicts are probably the result of a recent merge of my dispatch version work. Possibly it would have been better to merge Dennis' PR first. I would try to fix these, but I have no write permissions on this branch. ;-( |
|
Also, nc_test4/tst_filterparser.c fails address sanitizer checks due to a memory leak in the test. It's fixed by inserting this as line 251 of tst_filterparser.c:
I have fixed this on my branch and am running it through my CI system now, but I'll wait to put up any PR... |
|
ok, thanks for the fix. |
|
Wait, we seem to be out of synch. There is no variable named pfs |
|
OK, why don't you get these changes in and then I can rerun my CI to see if there is anything to fix... |
|
If it's easier to roll back the last two of my PRs that were merged, then this PR could be merged, probably without conflict, and I can reapply my changes. I'm happy to do this if it will help... |
|
I fixed the conflicts. Why is it not rebuilding? |
|
When you fixed the conflicts, did you do a git add for the conflicted files? Alternatively, you can use the github gui to resolve the conflicts. |
|
Ok, I think this PR is ready to go. |
|
In order to compile this branch for parallel builds, I had to change line 449 of libsrc4/nc4var.c from:
However, this then failed the szip parallel write test in nc_test4/tst_parallel5.c. I will append output of the logging. (However, I later found the fix - see next message.) In order to run this test you must build HDF5 for parallel with szip, then you must build netcdf-c with that HDF5. (Recall that for the HDF5 configure you must use --enable-parallel, and for the netcdf-c configure you must use --enable-parallel-tests.) This also failed on your branch before merging in recent changes, so this is not a result of the recent merge actions. I don't know what is going on with this error, I'm still trying to figure it out... |
|
OK I found the problem. In hdf5filter.c I had to change the code around line 307 from: to: |
|
Ok, I pushed Ed's fix for the parallel filters problem. |
|
Testing now, thanks Dennis! |
|
Unfortunately the fix for nc4var.c above was not applied, so parallel builds on master are now broken. ;-) I will have a PR up shortly with a PR. @WardF I suggest you add a parallel build to travis to catch this in future. We do build PIO with MPICH on travis, so it can be done... |
|
Whar nc4var fix are you referring to? |
|
Oh, you mean the hdf5filter fix. There is a commit for it: |
|
No, there were two fixes, one in hdf5filter.c and one in nc4var.c. I have a PR up now with the nc4var.c change - it's just one line of code. I don't yet know what is happening with my cmake tests, I'll take a look... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support has been added for multiple filters per variable. This
affects a number of components in netcdf. The new APIs are
documented in NUG/filters.md.
The primary changes are:
about the first defined filter.
an extended format for specifying a sequence of filters.
The general form is __<filter>|<filter>..._.
filters associated with a variable using the above new format.
for filters known to the netcdf library, which in turn is taken
from the HDF5 filter registration page.
is returned if an attempt is made to access an unknown filter.
to handle all of the filter functions.
Notes:
Misc. changes: