Added HDF5 compression to EmpCylSL disk cache files by The9Cat · Pull Request #183 · EXP-code/EXP

The9Cat · 2025-12-05T18:44:42Z

The code changes are very simple. On small builds, the compression factor is ~1.2. Might be better for larger cache files.

Copilot

Pull request overview

This PR adds HDF5 compression support to EmpCylSL disk cache files to reduce file sizes. The compression is achieved using the HDF5 Deflate algorithm with a default compression level of 7, along with optional shuffle filter and Szip compression support.

Adds three protected member variables for configuring HDF5 compression settings (H5compress, H5shuffle, H5szip)
Implements compression in the WriteH5Cache() method by creating a DataSetCreateProps object with appropriate compression settings
Applies compression to all 8 cache datasets (potC, rforceC, zforceC, densC, potS, rforceS, zforceS, densS)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
include/EmpCylSL.H	Adds three protected member variables for HDF5 compression configuration (H5compress=7, H5shuffle=true, H5szip=false)
exputil/EmpCylSL.cc	Implements compression logic in WriteH5Cache() by configuring DataSetCreateProps with chunking, shuffle filter, and Deflate/Szip compression, then applies it to all 8 cache datasets

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

include/EmpCylSL.H

exputil/EmpCylSL.cc

include/EmpCylSL.H

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot · 2025-12-05T20:59:10Z

@The9Cat I've opened a new pull request, #184, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Add public setter for HDF5 compression parameters in EmpCylSL

…YAML configuration parameter; I can't see why someone would want to turn this off, but it's an optin

The9Cat · 2025-12-06T15:06:09Z

The last commit allows the user to change the compression level. E.g. setting compress: 0 turns compression off. Not sure why anyone would want that but one can.

HDF5 compression for EmpCylSL seems like a no -brainer to me. I can't see any downside to this PR. A 40% decrease in disk-space usage and the compression is handled automatically by the HDF5 libraries and h5py.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Cylinder.H

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-11T14:15:29Z

src/Cylinder.cc

+  // Set HDF5 compression parameters for cache files
+  if (conf["compress"]) {
+    unsigned compress = conf["compress"].as<unsigned>();
+    ortho->setH5Params(compress);
+  }


The Cylinder class only exposes the compress parameter, while the BiorthBasis Cylindrical class exposes both compress and shuffle parameters. This creates an inconsistent API between the two interfaces. Consider either adding shuffle support to Cylinder.cc (by adding "shuffle" to valid_keys and parsing it), or documenting why this parameter is only available in one interface.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-11T14:15:29Z

src/Cylinder.cc

+  // Set HDF5 compression parameters for cache files
+  if (conf["compress"]) {
+    unsigned compress = conf["compress"].as<unsigned>();
+    ortho->setH5Params(compress);
+  }


The new compression functionality lacks test coverage. Consider adding tests that verify: (1) compression can be enabled/disabled via the compress parameter, (2) compressed cache files can be read back correctly, (3) the shuffle parameter works as expected, and (4) validation of the compression level (0-9 range). The existing test file tests/Disk/cyl_basis.py could be extended to include these cases.

@copilot open a new pull request to apply changes based on this feedback

src/Cylinder.H

exputil/EmpCylSL.cc

Copilot · 2025-12-11T14:15:30Z

include/EmpCylSL.H

+  //! Set HDF5 compression parameters for cache files
+  //! @param compress Compression level (valid range: 0-9, where 0=no compression, 9=max compression)
+  //! @param shuffle Enable byte shuffle filter (default: true)
+  //! @param szip Enable szip compression (default: false)
+  void setH5Params(unsigned compress, bool shuffle=true, bool szip=false)
+  {
+    // Validate compression level (unsigned type prevents negative values)
+    if (compress > 9) {
+      if (myid==0) {
+	std::cout << "EmpCylSL: compression level " << compress
+		  << " out of range [0,9], using 9" << std::endl;
+      }
+      compress = 9;
+    }
+    H5compress = compress;
+    H5shuffle  = shuffle;
+    H5szip     = szip;
+  }


The current implementation makes szip and deflate compression mutually exclusive (only one can be active at a time), but this is not documented. The setH5Params method documentation should clarify that when szip=true, the compress parameter (which controls deflate/gzip compression level) is ignored. Consider adding documentation or a warning when both are configured.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-11T14:15:31Z

exputil/EmpCylSL.cc

+      // Chunking
+      unsigned long nx = NUMX + 1, ny = NUMY + 1;
+      dcpl.add(HighFive::Chunking({nx, ny}));
+      if (H5shuffle) dcpl.add(HighFive::Shuffle());


The shuffle filter is applied even when H5compress=0 (no compression). The shuffle filter reorders bytes to improve compressibility, but has no benefit and adds overhead when compression is disabled. Consider only applying the shuffle filter when compression is actually enabled (i.e., when H5compress > 0 or H5szip is true).

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-11T14:18:24Z

@The9Cat I've opened a new pull request, #187, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

[WIP] Address feedback on HDF5 compression implementation

…tion Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Add test coverage for HDF5 compression in EmpCylSL disk cache

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Copilot · 2025-12-11T14:34:13Z

@The9Cat I've opened a new pull request, #190, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Copilot · 2025-12-11T14:34:34Z

@The9Cat I've opened a new pull request, #191, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

…pression Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Only apply HDF5 shuffle filter when compression is enabled

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Only apply HDF5 shuffle filter when compression is enabled

Change setH5Params to use deflate when both szip and deflate compression are configured

The9Cat · 2025-12-11T16:23:53Z

This PR is done for now and ready for review. Main new features are:

Allow HDF5 gzip (deflate compression) in construction of disk-basis caches with or without bit shuffling. Bit shuffling tends to be a significant win so it is on by default.
Added sanity checking for user parameters (e.g. compression in [0, 9], tryng to use gzip and szip results in a warning and defaulting to gzip)
The API is consistent between BiorthBasis in pyEXP and Cylinder in EXP.
Tested reads and writes for different sized grids, etc.
Added a ctest test Python test script to check all of the user-controllable compression features

The9Cat · 2025-12-11T16:25:59Z

@michael-petersen No hurry on this. But work on this PR is completed on my end.

Add HDF5 compression to EmpCylSL disk cache files

56a7559

The9Cat requested a review from Copilot December 5, 2025 18:44

Copilot started reviewing on behalf of The9Cat December 5, 2025 18:45 View session

Copilot AI reviewed Dec 5, 2025

View reviewed changes

include/EmpCylSL.H Show resolved Hide resolved

exputil/EmpCylSL.cc Outdated Show resolved Hide resolved

include/EmpCylSL.H Show resolved Hide resolved

The9Cat and others added 2 commits December 5, 2025 14:40

Update exputil/EmpCylSL.cc

87cb7c6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update include/EmpCylSL.H

2d61ad0

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

The9Cat mentioned this pull request Dec 5, 2025

Consider adding HDF5 compression to EmpCylSL cache files #182

Open

Initial plan

7349fe3

Copilot AI mentioned this pull request Dec 5, 2025

Add public setter for HDF5 compression parameters in EmpCylSL #184

Merged

Copilot AI and others added 6 commits December 5, 2025 21:04

Add public setter for HDF5 compression parameters in EmpCylSL

02a96bc

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Add validation for compression level in setH5Params

a5eb863

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Add documentation for setH5Params method

fab96b8

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Merge pull request #184 from EXP-code/copilot/sub-pr-183

e7f8f8b

Add public setter for HDF5 compression parameters in EmpCylSL

Allow the compression level to be user settable using the 'compress' …

020b521

…YAML configuration parameter; I can't see why someone would want to turn this off, but it's an optin

Doc string updates only [no CI]

e8fb1a5

The9Cat requested a review from Copilot December 6, 2025 15:09

Copilot started reviewing on behalf of The9Cat December 6, 2025 15:09 View session

Copilot AI reviewed Dec 6, 2025

View reviewed changes

src/Cylinder.H Outdated Show resolved Hide resolved

The9Cat and others added 2 commits December 6, 2025 10:55

Update src/Cylinder.H

ceb51ad

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Allow the user to toggle the HDF5 bit shuffle option.

e0f6b65

The9Cat requested a review from Copilot December 11, 2025 14:09

Copilot started reviewing on behalf of The9Cat December 11, 2025 14:09 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

Initial plan

c410a77

Copilot AI mentioned this pull request Dec 11, 2025

Add shuffle parameter support to Cylinder class #187

Merged

Initial plan

1d68525

Copilot AI mentioned this pull request Dec 11, 2025

Add test coverage for HDF5 compression in EmpCylSL disk cache #188

Merged

Copilot AI and others added 11 commits December 11, 2025 14:25

Remove __pycache__ from git and update .gitignore

c2ce07a

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Complete API consistency improvements

d822cfa

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Merge branch 'hdf5compress' into copilot/sub-pr-183-again

9b37116

Address code review feedback: fix typo and add documentation reference

3295b99

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Apply shuffle filter only when compression is actually enabled

63e71d5

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Merge pull request #187 from EXP-code/copilot/sub-pr-183-again

1072aa3

[WIP] Address feedback on HDF5 compression implementation

Add explicit compression check for shuffle filter in sample stats sec…

447734e

…tion Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Merge pull request #188 from EXP-code/copilot/sub-pr-183-another-one

f8d281b

Add test coverage for HDF5 compression in EmpCylSL disk cache

Use explicit H5compress > 0 checks for consistency and clarity

1afbfdc

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Simplify shuffle filter conditions and improve consistency

a02348f

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Initial plan

a6b232f

Copilot AI mentioned this pull request Dec 11, 2025

Change setH5Params to use deflate when both szip and deflate compression are configured #190

Merged

Copilot AI and others added 2 commits December 11, 2025 14:34

Clarify comment for Deflate-only compression section

6b7271e

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Initial plan

13f867e

Copilot AI mentioned this pull request Dec 11, 2025

Only apply HDF5 shuffle filter when compression is enabled #191

Merged

Copilot AI and others added 10 commits December 11, 2025 14:35

Final update to PR description with complete summary

ae5b00f

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Add documentation and warning for mutually exclusive szip/deflate com…

4a9fa02

…pression Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Merge pull request #189 from EXP-code/copilot/sub-pr-183-yet-again

a4f1c09

Only apply HDF5 shuffle filter when compression is enabled

Apply shuffle filter only when compression is enabled

fa6ad6d

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Change behavior to use deflate when both szip and deflate are configured

5712475

Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>

Merge branch 'hdf5compress' into copilot/sub-pr-183-another-one

9bef58a

Merge pull request #191 from EXP-code/copilot/sub-pr-183-another-one

ecf706a

Only apply HDF5 shuffle filter when compression is enabled

Merge pull request #190 from EXP-code/copilot/sub-pr-183-again

e2c0a88

Change setH5Params to use deflate when both szip and deflate compression are configured

Removed old config pararameters to suppress all the warnings

71bf386

Add more comments to test script

2cdc8a9

The9Cat assigned michael-petersen Dec 11, 2025

Conversation

The9Cat commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 5, 2025

Uh oh!

The9Cat commented Dec 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

The9Cat Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

The9Cat Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

The9Cat Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

The9Cat Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

The9Cat Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Dec 11, 2025

Uh oh!

Copilot AI commented Dec 11, 2025

Uh oh!

Copilot AI commented Dec 11, 2025

Uh oh!

The9Cat commented Dec 11, 2025

Uh oh!

The9Cat commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants