Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Binary license notices are not installed, and several standalone build and test configurations contain functional errors.
Review effort: Balanced
Findings: 8
Open (8)
What changed in this PR
Adds the SZ3 3.4.0 lossy-compression filter (ID 32024), including source acquisition, plugin packaging, examples, tests, documentation, and CI coverage.
Changes:
- Builds
h5sz3from the bundled or Git-fetched SZ3 release. - Adds filter examples, golden-file tests, and distributable example infrastructure.
- Integrates SZ3 into repository configuration, documentation, and workflows.
| File | Description |
|---|---|
SZ3/src/CMakeLists.txt |
Builds and installs the plugin. |
SZ3/README.md |
Documents build and usage. |
SZ3/example/testfiles/ud_convert.h5repack_layout.h5.tst |
Adds repack reference output. |
SZ3/example/testfiles/h5repack_layout.h5-ud_convert.ddl |
Adds filtered-layout reference. |
SZ3/example/testfiles/h5ex_d_sz3.tst |
Adds example output reference. |
SZ3/example/test/testCM.sh |
Adds CMake-install test script. |
SZ3/example/test/test.sh |
Adds autotools-install test script. |
SZ3/example/h5ex_d_sz3.c |
Demonstrates SZ3 round-trip validation. |
SZ3/example/CMakeLists.txt |
Configures example tests. |
SZ3/CTestConfig.cmake |
Configures dashboard testing. |
SZ3/config/toolchain/pgi.cmake |
Adds PGI toolchain. |
SZ3/config/toolchain/mingw64.cmake |
Adds MinGW toolchain. |
SZ3/config/toolchain/intel.cmake |
Adds oneAPI toolchain. |
SZ3/config/toolchain/icc.cmake |
Adds classic Intel toolchain. |
SZ3/config/toolchain/gcc.cmake |
Adds GCC toolchain. |
SZ3/config/toolchain/crayle.cmake |
Adds Cray toolchain. |
SZ3/config/toolchain/clang.cmake |
Adds Clang toolchain. |
SZ3/config/toolchain/build32.cmake |
Adds 32-bit toolchain settings. |
SZ3/config/toolchain/aarch64.cmake |
Adds AArch64 toolchain. |
SZ3/config/cmake/SignPackageFiles.cmake |
Adds package-signing support. |
SZ3/config/cmake/README.txt.cmake.in |
Adds package README template. |
SZ3/config/cmake/NSIS.InstallOptions.ini.in |
Adds NSIS options. |
SZ3/config/cmake/HDFPLoptions.cmake |
Adds build-option overrides. |
SZ3/config/cmake/H5SZ3Macros.cmake |
Fetches and configures SZ3. |
SZ3/config/cmake/h5sz3-config.cmake.in |
Adds package configuration template. |
SZ3/config/cmake/h5sz3-config-version.cmake.in |
Adds package version checks. |
SZ3/config/cmake/H5PLTests.c |
Adds configuration probes. |
SZ3/config/cmake/H5PL_Examples.cmake.in |
Adds installed-example driver. |
SZ3/config/cmake/grepTest.cmake |
Adds output-checking helper. |
SZ3/config/cmake/distribution.entitlements |
Adds macOS signing entitlements. |
SZ3/config/cmake/CTestScript.cmake |
Adds dashboard build script. |
SZ3/config/cmake/CTestCustom.cmake |
Adds CTest customizations. |
SZ3/config/cmake/ConfigureChecks.cmake |
Adds platform checks. |
SZ3/config/cmake/config.h.in |
Adds generated configuration header. |
SZ3/config/cmake/cacheinit.cmake |
Adds cache defaults. |
SZ3/config/cmake/binex/Using_CMake.txt |
Documents packaged examples. |
SZ3/config/cmake/binex/example/testfiles/ud_convert.h5repack_layout.h5.tst |
Packages repack output. |
SZ3/config/cmake/binex/example/testfiles/h5repack_layout.h5-ud_convert.ddl |
Packages layout reference. |
SZ3/config/cmake/binex/example/testfiles/h5ex_d_sz3.tst |
Packages example output. |
SZ3/config/cmake/binex/example/test/testCM.sh |
Packages CMake test script. |
SZ3/config/cmake/binex/example/test/test.sh |
Packages autotools test script. |
SZ3/config/cmake/binex/example/h5ex_d_sz3.c |
Packages the example source. |
SZ3/config/cmake/binex/example/CMakeLists.txt |
Builds packaged examples. |
SZ3/config/cmake/binex/CTestConfig.cmake |
Configures packaged tests. |
SZ3/config/cmake/binex/config/toolchain/pgi.cmake |
Packages PGI settings. |
SZ3/config/cmake/binex/config/toolchain/mingw64.cmake |
Packages MinGW settings. |
SZ3/config/cmake/binex/config/toolchain/icc.cmake |
Packages Intel settings. |
SZ3/config/cmake/binex/config/toolchain/gcc.cmake |
Packages GCC settings. |
SZ3/config/cmake/binex/config/toolchain/crayle.cmake |
Packages Cray settings. |
SZ3/config/cmake/binex/config/toolchain/clang.cmake |
Packages Clang settings. |
SZ3/config/cmake/binex/config/toolchain/build32.cmake |
Packages 32-bit settings. |
SZ3/config/cmake/binex/config/toolchain/aarch64.cmake |
Packages AArch64 settings. |
SZ3/config/cmake/binex/config/cmake/HDFPluginMacros.cmake |
Supports packaged HDF5 discovery. |
SZ3/config/cmake/binex/config/cmake/grepTest.cmake |
Packages output checks. |
SZ3/config/cmake/binex/config/cmake/CTestCustom.cmake |
Packages CTest settings. |
SZ3/config/cmake/binex/config/cmake/cacheinit.cmake |
Packages cache defaults. |
SZ3/config/cmake/binex/CMakePresets.json |
Adds packaged-example presets. |
SZ3/config/cmake/binex/CMakeLists.txt |
Defines packaged-example project. |
SZ3/config/cmake-presets/hidden-presets.json |
Adds shared SZ3 presets. |
SZ3/config/CacheURLs.cmake |
Defines SZ3 source locations. |
SZ3/CMakeLists.txt |
Configures the SZ3 subproject. |
SZ3/Additional_Legal/zstd-LICENSE |
Records bundled Zstd license. |
SZ3/Additional_Legal/copyright-and-BSD-license.txt |
Records SZ3 license terms. |
libs/SZ3-3.4.0.tar.gz |
Vendors the SZ3 release archive. |
docs/RegisteredFilterPlugins.md |
Corrects the SZ3 license link. |
docs/PluginLibraries.txt |
Documents filter ID and parameters. |
config/CacheURLs.cmake |
Adds repository-wide SZ3 URLs. |
CMakePresets.json |
Adds SZ3 preset variables. |
CMakeLists.txt |
Adds the conditional SZ3 filter option. |
.github/workflows/main-package-managers-win.yml |
Disables unavailable packaged SZ3. |
.github/workflows/main-package-managers-ubuntu.yml |
Disables unavailable packaged SZ3. |
.github/workflows/main-package-managers-mac.yml |
Disables unavailable packaged SZ3. |
.github/workflows/main-fetchcontent-win.yml |
Enables fetched SZ3 on Windows. |
.github/workflows/main-fetchcontent-ubuntu.yml |
Enables fetched SZ3 on Ubuntu. |
.github/workflows/main-fetchcontent-mac.yml |
Enables fetched SZ3 on macOS. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| set (SZ3_BRANCH ${SZ3_GIT_BRANCH}) | ||
| elseif (H5PL_ALLOW_EXTERNAL_SUPPORT MATCHES "TGZ") | ||
| if (NOT H5PL_COMP_TGZPATH) | ||
| set (H5PL_COMP_TGZPATH ${H5SZ3_SOURCE_DIR}) |
There was a problem hiding this comment.
This is the same default as the other filters (ZSTD sets H5PL_COMP_TGZPATH to its own source directory too): the top-level build passes H5PL_COMP_TGZPATH=libs through the presets, and a standalone configure passes it explicitly, as the README says.
| set(TOOLCHAIN_PREFIX aarch64-linux-gnu) | ||
| set(ANDROID_NDK /opt/android-ndk-linux) | ||
| set (CMAKE_SYSTEM_NAME Android) | ||
| set (CMAKE_ANDROID_ARCH_ABI x86_64) | ||
| #set (CMAKE_ANDROID_STANDALONE_TOOLCHAIN ${ANDROID_NDK}/build/cmake/andriod.toolchain.cmake) |
There was a problem hiding this comment.
This file is the shared template copy; ZSTD, BLOSC2 and ZFP carry the same file unchanged, so I kept SZ3's identical to them. If it should be fixed, that is a change for every filter's copy, better done in a separate PR.
| srcdir=.. | ||
| builddir=. | ||
| verbose=yes | ||
| nerrors=0 |
There was a problem hiding this comment.
This file is the shared template copy; ZSTD, BLOSC2 and ZFP carry the same file unchanged, so I kept SZ3's identical to them. If it should be fixed, that is a change for every filter's copy, better done in a separate PR.
| srcdir=.. | ||
| builddir=. | ||
| verbose=yes | ||
| nerrors=0 |
There was a problem hiding this comment.
This file is the shared template copy; ZSTD, BLOSC2 and ZFP carry the same file unchanged, so I kept SZ3's identical to them. If it should be fixed, that is a change for every filter's copy, better done in a separate PR.
| set (CMAKE_C_COMPILER cc) | ||
| set (CMAKE_Fortran_COMPILER ftn) |
There was a problem hiding this comment.
This file is the shared template copy; ZSTD, BLOSC2 and ZFP carry the same file unchanged, so I kept SZ3's identical to them. If it should be fixed, that is a change for every filter's copy, better done in a separate PR.
| srcdir=.. | ||
| builddir=. | ||
| verbose=yes | ||
| nerrors=0 |
There was a problem hiding this comment.
This file is the shared template copy; ZSTD, BLOSC2 and ZFP carry the same file unchanged, so I kept SZ3's identical to them. If it should be fixed, that is a change for every filter's copy, better done in a separate PR.
| srcdir=.. | ||
| builddir=. | ||
| verbose=yes | ||
| nerrors=0 |
There was a problem hiding this comment.
This file is the shared template copy; ZSTD, BLOSC2 and ZFP carry the same file unchanged, so I kept SZ3's identical to them. If it should be fixed, that is a change for every filter's copy, better done in a separate PR.
| LIBRARY DESTINATION ${H5SZ3_INSTALL_LIB_DIR} COMPONENT libraries | ||
| ARCHIVE DESTINATION ${H5SZ3_INSTALL_LIB_DIR} COMPONENT libraries | ||
| RUNTIME DESTINATION ${H5SZ3_INSTALL_BIN_DIR} COMPONENT libraries | ||
| ) |
There was a problem hiding this comment.
Fixed: the plugin install now puts copyright-and-BSD-license.txt and zstd-LICENSE in <docdir>/SZ3.
|
Thanks for the thorough work on this. But we're taking a different approach than we did for the previous filters, so we would like to use the ZFP model (ZFP/) as an example. The approach Bring SZ3 in as a git subtree, pinned to the release:
The rule: we never edit the files under SZ3/SZ3/. If the upstream build needs a change to fit here, it goes to szcompressor/SZ3 first, and we pull it in with git subtree pull. ZFP/README.md and ZFP/UPDATING_ZFP_SUBTREE.md are good templates for the SZ3 docs. |
git-subtree-dir: SZ3/SZ3 git-subtree-split: 6141ed7879ad4812fa1e7d1c77e3459bd4baffc6
Build the H5Z-SZ3 filter with SZ3's own CMake from the SZ3 v3.4.0 git subtree in SZ3/SZ3, following the ZFP/ layout: SZ3/CMakeLists.txt is a thin wrapper that adds SZ3 with BUILD_H5Z_FILTER and its bundled Zstd, installs the plugin to lib/plugin through H5Z_SZ3_PLUGIN_INSTALL_DIR, copies it into plugins/ for testing and aliases hdf5sz3 to h5sz3. SZ3 needs no external library, so the filter also builds with H5PL_ALLOW_EXTERNAL_SUPPORT=NO. Includes an example program, h5repack/h5dump tests, the SZ3 and Zstd license notices installed with the plugin, the docs for the subtree, and ENABLE_SZ3 in the workflows.
|
@brtnfld Thanks, reworked as you described. The branch is now two parts on top of master: the subtree add (squash and merge commits) and one commit (0cb0085) with the wrapper, example, tests, docs and workflows.
Testing: HDF5 2.2.0, GCC 13.3, CMake 3.28, Release, fresh build dirs, no network.
Differences from ZFP:
|
592df793 CMake: find the system Zstd with find_library; a static SZ3c links on Windows (HDFGroup#184) git-subtree-dir: SZ3/SZ3 git-subtree-split: 592df793eb596d952242aa9fec9cbe5e68782aae
|
@brtnfld the subtree is now at the final v3.4.0 tag (592df79): one |
Add the SZ3 filter (plugin ID 32024)
Adds SZ3, the error-bounded lossy compressor registered as filter 32024, as a plugin built and tested in this repository, following the ZFP model.
Layout
SZ3/SZ3/: SZ3 v3.4.0 as a git subtree (git subtree add --prefix=SZ3/SZ3 https://github.com/szcompressor/SZ3.git v3.4.0 --squash,git-subtree-split592df793eb596d952242aa9fec9cbe5e68782aae). Nothing under it is edited; updates come withgit subtree pull(seeSZ3/UPDATING_SZ3_SUBTREE.md).SZ3/CMakeLists.txt: a thin wrapper modeled onZFP/CMakeLists.txt. It builds the filter with SZ3's own CMake (tools/H5Z-SZ3,BUILD_H5Z_FILTER=ON), setsH5Z_SZ3_PLUGIN_INSTALL_DIRtolib/plugin, copies the plugin intoplugins/for the tests (copy_sz3_plugin), aliasesh5sz3to SZ3'shdf5sz3target and installs the license notices toshare/SZ3.SZ3/example/:h5ex_d_sz3.cand its tests;SZ3/Additional_Legal/: SZ3's BSD license and the license of the Zstd it bundles;SZ3/README.md,SZ3/UPDATING_SZ3_SUBTREE.md.CMakeLists.txt:FILTER_OPTION (SZ3), off on MinGW, withDISABLE_H5PL_ENCODER(H5Z-SZ3 has no decode-only build) and on CMake older than 3.19 (SZ3's minimum). The filter needs noGIT/TGZsource and builds withH5PL_ALLOW_EXTERNAL_SUPPORT=NO.-DENABLE_SZ3=ON/OFFnext toENABLE_BITROUND;./SZ3/SZ3added to the clang-format excludes next to./ZFP/H5Z-ZFP.docs/PluginLibraries.txt: an SZ3 section.docs/RegisteredFilterPlugins.md: the SZ3 entry's license link now points at SZ3 instead of SZ (SZ2).How it differs from ZFP
SZ3_USE_BUNDLED_ZSTD=ON: SZ3's vendored Zstd is compiled into the plugin with hidden symbols, so the plugin needs neither network nor a system Zstd, and its only non-system dependency is HDF5.SZ3_INSTALL=ON, because SZ3 runs its install rules (including the copy toH5Z_SZ3_PLUGIN_INSTALL_DIR) only then.cmake --installtherefore also installs SZ3's development files (headers,libhdf5sz3,libsz3_zstd.a,lib/cmake/SZ3), much as H5Z-ZFP installs its library and headers. If only the plugin should be installed, that needs an option in SZ3, which we can add upstream and pull in.libhdf5sz3.so.3.4.0, with its links); only the CMake target is aliased toh5sz3.project(SZ3 VERSION ...)inSZ3/SZ3/CMakeLists.txt. SZ3's own tests are not built (SZ3 adds them only as a top-level project).Tests
h5ex_d_sz3.cwrites a 32x64 float dataset withH5Pset_filter(dcpl, 32024, H5Z_FLAG_MANDATORY, 0, NULL)(SZ3's defaults:ALGO_INTERP_LORENZO, absolute bound 1e-3), reopens the file and fails if any value is off by more than the bound. The tests, as for the other filters:h5ex_d_sz3,H5DUMP-h5ex_d_sz3,H5SZ3_UD-ud_convert,H5SZ3_UD-h5dump-ud_convert(the h5dump comparisons maskPARAMS, as ZFP's do).Linux x86_64, GCC 13.3, CMake 3.28, HDF5 2.2.0, Release, fresh build directories, no network:
TGZ), all filters onH5PL_ALLOW_EXTERNAL_SUPPORT=NO(filters without system libraries off)SZ3/configured on its ownIn each, the plugin lands in
plugins/, and aftercmake --installit is inlib/pluginwith the SZ3 and Zstd notices inshare/SZ3.Questions for reviewers
config/cmake/binex) has a ZFP example but no SZ3 one yet; add it here or in a follow-up?community/sz3/README.mdis still the empty placeholder; remove it, or point it here?