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

re-implement the nc_def_var_szip() function, including for parallel I/O #1589

Merged
merged 28 commits into from
Jan 22, 2020
Merged

re-implement the nc_def_var_szip() function, including for parallel I/O #1589

merged 28 commits into from
Jan 22, 2020

Conversation

edwardhartnett
Copy link
Contributor

@edwardhartnett edwardhartnett commented Jan 6, 2020

Fixes #1546
Fixes #1602

In this PR I re-add the old nc_def_var_szip() function.

The capability of using szip compression, when supported by HDF5, was already present in the library (though not mentioned in the documentation). This PR adds the old function to help users achieve this.

The code has also been changed so that parallel I/O works with szip. This is fully supported by HDF5, we just need to let it work.

I have deferred the question of whether this function should be added to the dispatch table. @DennisHeimbigner, do you agree that it should be?

After this PR is merged I will add support in the Fortran APIs.

This PR also includes open PR #1582

My travis runs are all failing because of the remote access tests. I believe you guys are working on that problem...

@gsjaardema
Copy link
Contributor

What is the recommended method for a NetCDF client to check whether the library they are linking too support SZIP compression? Should I check based on version, or ?. I don't seen anything in netcdf_meta.h I can use or in netcdf.h or nc-config

include/nc4internal.h Outdated Show resolved Hide resolved
@DennisHeimbigner
Copy link
Collaborator

Of course you can try to invoke nc_def_var_szip and it will fail.
But I assume that you want a compile time test. AFAIK I don't think
we have one. (Nor do we have one for zip, butthat is assumed to be
always available).
Not sure we should have a compile time test because from my point
of view, szip us no different than any other filter.

@DennisHeimbigner
Copy link
Collaborator

Ed asks if szip should be in the dispatch table.
As I state above, I view szip like any other filter
so I would actually run szip thru nc_def_var_filter
with (internally) a special check for the szip filter id.
Similarly, I would put extern for nc_def_var_szip
in include/netcdf_filter.h.

@DennisHeimbigner
Copy link
Collaborator

I think Dave is right; the ifdef flags for szip are wrong.
The flag in the HDF5 public header appears to be H5_HAVE_FILTER_SZIP

@edwardhartnett
Copy link
Contributor Author

edwardhartnett commented Jan 6, 2020

@DennisHeimbigner actually szip is not like any other filter (except zlib). For example, the existing code handles setting of the szip filter with nc_def_var_filter() differently from setting other filters. The szip code is checked for, and H5Pset_szip() is called instead of H5Pset_filter().

Also, netcdf filters do not currently allow parallel, so if I set this through nc_def_var_filter() it would disallow parallel I/O, which works, and works very well. It's necessary, to achieve performance. So we definitely don't want to limit szip (or other filters) to sequential-only builds.

Also, also, the current filter implementation seems to only allow one filter. We are intending to add a bit-grooming filter for HDF5, and if we want to use that filter with szip we need szip to be handled separately, or filters to handle multiple filters.

Our config.h has:

/* Define to 1 if you have the `H5Z_SZIP' function. */
#define HAVE_H5Z_SZIP 1

Do we think I should be using something else?

@edwardhartnett
Copy link
Contributor Author

Also @DennisHeimbigner WRT moving prototype to netcdf_filter.h: I will do so.

@WardF WardF self-assigned this Jan 8, 2020
@gsjaardema
Copy link
Contributor

@edwardhartnett

Our config.h has:

/* Define to 1 if you have the `H5Z_SZIP' function. */
#define HAVE_H5Z_SZIP 1

Do we think I should be using something else?

I don't think this will allow me to ifdef my application trying to call nc_def_var_szip() since the define (and the similar NC_HAS_SZIP) both existed prior to this PR. I need something that changes based on this PR such that I know that the library contains the API function nc_def_var_szip()

@edwardhartnett
Copy link
Contributor Author

@gsjaardema OK I see what you mean. Standby and I will add. Also I need to resolve the conflict, and see if Travis test are back up and running again...

@edwardhartnett
Copy link
Contributor Author

@gsjaardema thanks for pointing this out. I have added NC_HAS_SZIP_WRITE and NC_HAS_PAR_FILTERS to netcdf_meta.h, and similar fields to libnetcdf.settings.

@edwardhartnett
Copy link
Contributor Author

@WardF this PR is now ready to merge. Thanks!

@WardF WardF merged commit aadd5a2 into Unidata:master Jan 22, 2020
@WardF
Copy link
Member

WardF commented Jan 22, 2020

Looks great, thanks!

@edwardhartnett edwardhartnett deleted the ejh_szip branch February 26, 2020 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants