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

Use bitshuffle lz4 decompression for DLS Eiger images. #236

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jbeilstenedmands
Copy link

Implements a faster Eiger decompression using bshuf_decompress_lz4 from bitshuffle. The changes are restricted to only affect FormatNexusEigerDLS.py as I believe we can only make certain assumptions about the data structure for DLS written data.

Outstanding issues:

  • this doesn't compile properly on linux - you can see in the CI jobs: build/lib/dxtbx_format_nexus_ext.so: undefined symbol: H5DOread_chunk. I'm hoping someone has suggestions on the fix for this?
  • this uses the c code for bitshuffle. The license for this code is MIT (https://github.com/kiyo-masui/bitshuffle/blob/master/LICENSE) so I think we're fine to use it, but I'm not sure how we are meant to include this in our license. Also I have just copied the files in here, which feels like the dumb way to do this, but is there a better way?

Testing on my local machine, this reduced the runtime by ~25% for each call to imageset.get_raw_data(i), I would like to test on the DLS systems also.

@jbeilstenedmands jbeilstenedmands changed the title Define image_as_flex function using lz4 decompression Use bitshuffle lz4 decompression for DLS Eiger images. Oct 14, 2020
@jbeilstenedmands
Copy link
Author

So I have removed the bitshuffle files and added the package to the ci-conda-env.
For developers, this needs a conda install -c conda-forge bitshuffle and then seems to work for me.

@Anthchirp
Copy link
Member

as an aside: the bitshuffle package is not currently built on Windows -> conda-forge/bitshuffle-feedstock#18

@graeme-winter
Copy link
Collaborator

as an aside: the bitshuffle package is not currently built on Windows -> conda-forge/bitshuffle-feedstock#18

We should probably subclass the Format...DLS.py to include an import bitshuffle in the understand() method so we have a graceful fallback if $user does not have the library for whatever reason.

Or some equivalent for the locally compiled code.

But it would mean we have a nice fallback.

@Anthchirp
Copy link
Member

further: will need to add bitshuffle dependency to the cctbx bootstrap etc.

Also: wasn't import bitshuffle problematic with omptbx iirc?

@graeme-winter
Copy link
Collaborator

import bitshuffle is problematic as by default it is compiled with openMP - however I think / suspect in this context using the C code would not intersect with omptbx

@graeme-winter
Copy link
Collaborator

@jbeilstenedmands trying to build now to see how this works out

Base automatically changed from master to main March 1, 2021 10:53
@rjgildea
Copy link

rjgildea commented May 4, 2023

Is this still relevant? At least the format class FormatNexusEigerDLS hasn't existed for some time...

@graeme-winter
Copy link
Collaborator

Is this still relevant? At least the format class FormatNexusEigerDLS hasn't existed for some time...

This PR is probably obsolete, but the philosophy behind it is probably valid.

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

Successfully merging this pull request may close these issues.

4 participants