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

Enable shuffle filter #100

Closed
richli opened this issue Feb 15, 2023 · 5 comments · Fixed by #102
Closed

Enable shuffle filter #100

richli opened this issue Feb 15, 2023 · 5 comments · Fixed by #102

Comments

@richli
Copy link
Contributor

richli commented Feb 15, 2023

The nc_def_var_deflate() function has an argument to enable the HDF5 shuffle filter, which sometimes help improve compression ratios.

This function is called by the compression() method for a VariableMut and currently, the shuffle value is hard-coded to false. Could this setting be exposed? For example:

pub fn compression(&mut self, deflate_level: nc_type, shuffle: bool) -> Result<()> {}
@magnusuMET
Copy link
Member

That sounds like a great idea. Would you like to open a PR implementing this?

@lnicola
Copy link
Member

lnicola commented Feb 16, 2023

If we're (well, you're) breaking the API anyway, should this be something like .compression(Compression::deflate().level(6).shuffle(false))??

It looks like other compression methods are available (like blosc), but not supported in the bindings. My proposed API is not the easiest to implement, but please ignore that. My point is that shuffling seems (?) to be compression-dependent, so a future compression method might not offer shuffling.

@magnusuMET
Copy link
Member

That could be a good way to do it. I would like to revamp the API as netcdf-c has gotten multiple filters now, maybe we should have a VariableBuilder?

@richli
Copy link
Contributor Author

richli commented Feb 16, 2023

Yes, I'd be open to submitting a PR for the simple change, but I don't have time right now for something more involved like the API for a VariableBuilder. But I agree that's the way to go!

@magnusuMET
Copy link
Member

I am OK with breaking changes, there is quite a lot of unreleased code since v0.7.0 which requires more substantial updates for the library users. A quick and dirty fix by making compression take the shuffle is more than enough for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants