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

Expose c-blosc's bitshuffle capability (fix #87) #88

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Expose c-blosc's bitshuffle capability (fix #87) #88

merged 1 commit into from
Feb 1, 2022

Conversation

david-macmahon
Copy link
Contributor

Shuffling is now a choice between no shuffling, byte shuffling, or bit
shuffling. This changes the type of the shuffle keyword in compress
from Bool to its supertype Integer. The previous "no shuffling"
(shuffle=false) and "byte shuffling" (shuffle=true) options map
cleanly onto the Integer assignments used by the Blosc library. These
constants have been added to the Blosc module:

  • NOSHUFFLE (0) means do no shuffling (same as the old false)
  • SHUFFLE (1) means do byte shuffling (same as the old true)
  • BITSHUFFLE (1) means do bit shuffling (not previously supported)

The shuffled field in CompressionInfo has also been widened to
Integer to allow it to properly reflect all types of shuffling and the
compressor_info function properly decodes the flags to set the
shuffled field appropriately.

The tests have been updated to exercise all three shuffle modes.

Shuffling is now a choice between no shuffling, byte shuffling, or bit
shuffling.  This changes the type of the `shuffle` keyword in `compress`
from `Bool` to its supertype `Integer`.  The previous "no shuffling"
(`shuffle=false`) and "byte shuffling" (`shuffle=true`) options map
cleanly onto the `Integer` assignments used by the Blosc library.  These
constants have been added to the Blosc module:

- `NOSHUFFLE` (0) means do no shuffling (same as the old `false`)
- `SHUFFLE` (1) means do byte shuffling (same as the old `true`)
- `BITSHUFFLE` (1) means do bit shuffling (not previously supported)

The `shuffled` field in `CompressionInfo` has also been widened to
`Integer` to allow it to properly reflect all types of shuffling and the
`compressor_info` function properly decodes the flags to set the
`shuffled` field appropriately.

The tests have been updated to exercise all three shuffle modes.
@musm musm merged commit 0825a84 into JuliaIO:master Feb 1, 2022
@musm
Copy link
Member

musm commented Feb 3, 2022

Hmm is this backward compatible? I.e. can we still pass in true and have it default to 1? Otherwise we might have to release a mitigating patch for backwards compatbility

@musm
Copy link
Member

musm commented Feb 3, 2022

I've yanked the patch release, since this isn't backward compatible. I'll likely need to just make a new minor release and we need deprecation for the change.

@musm
Copy link
Member

musm commented Feb 3, 2022

Instead of deprecation, we can also just re-add the existing method. It's kind of code smell, but would at least support the existing interface and add additional functionality without deprecation.

I don't have a strong preference either way, but thought I'd mention this too.

@david-macmahon
Copy link
Contributor Author

I think it is backwards compatible. The tests for shuffle now test the following 5 value: NOSHUFFLE, SHUFFLE, BITSHUFFLE, true, false. Fortunately, NOSHUFFLE == 0 == false and SHUFFLE == 1 == true. Old code that used true/false should work seamlessly with the new code. The shuffle keyword was widened from Bool to Integer and Bool <: Integer.

@musm
Copy link
Member

musm commented Feb 4, 2022

Perfect, when I took another look I think I saw Int and got worried, thankfully it's Integer

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 this pull request may close these issues.

2 participants