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

Move compression and decompression routines into dxtbx #228

Merged
merged 17 commits into from
Oct 14, 2020

Conversation

graeme-winter
Copy link
Collaborator

3rd time lucky...

@graeme-winter graeme-winter marked this pull request as draft October 3, 2020 12:44
@graeme-winter
Copy link
Collaborator Author

Selecting draft appears to reset target ...

@graeme-winter graeme-winter changed the title First stab at moving compression in Move compression and decompression routines into dxtbx Oct 3, 2020
@graeme-winter graeme-winter marked this pull request as ready for review October 3, 2020 14:26
@graeme-winter
Copy link
Collaborator Author

Question to reviewers: would it aid readability to express e.g.

    if ((-32767 <= delta) && (delta < 32768)) {
      s = (short)delta;
      b = ((u_s *)&s)[0].b;

      if (!le) {
        byte_swap_short(b);
      }

      packed.push_back(b[0]);
      packed.push_back(b[1]);
      current += delta;
      continue;
    }

in hex?

@graeme-winter
Copy link
Collaborator Author

Attempts to fix SConscript in keeping with dials one have now completely broken it \o/

Will revisit Monday

@graeme-winter
Copy link
Collaborator Author

Attempts to fix SConscript in keeping with dials one have now completely broken it \o/

Will revisit Monday

Wait, what? This fixed it 🤔

@graeme-winter
Copy link
Collaborator Author

OK, something weird here.

The tests were failing for me, passing in here.

I thought was my build being skewed so I have just made a clean bootstrap and ... still fails.

Lots of

E       Boost.Python.ArgumentError: Python argument types in
E           Scan.__init__(Scan, tuple, tuple, double, double, int, bool)
E       did not match C++ signature:
E           __init__(boost::python::api::object, scitbx::vec2<int> image_range, scitbx::vec2<double> oscillation, scitbx::af::shared<double> exposure_times, scitbx::af::shared<double> epochs, int batch_offset=0, bool deg=True)
E           __init__(boost::python::api::object, scitbx::vec2<int> image_range, scitbx::vec2<double> oscillation, int batch_offset=0, bool deg=True)
E           __init__(_object*, dxtbx::model::Scan)
E           __init__(_object*)

why is this not failing centrally?

@Anthchirp
Copy link
Member

covered by tests skipped here because dials_regression?

@graeme-winter
Copy link
Collaborator Author

I have a clean build with no dials regression, so that is quite odd. Suspect it would fail in a clean build, but maybe the route for bootstrapping to where it got to with incremental builds is such that it has something built & linked.

Guess no-op changes to force rebuild could flush out some errors?

@Anthchirp
Copy link
Member

You can force flushed cache builds by resetting the date in line 2 of .azure-pipelines.yml

This branch was branched before the Azure builds were set up, so you
would only get PR builds, and not push builds. Updating branch to enable
push builds.
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #228 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   45.36%   45.36%   -0.01%     
==========================================
  Files         229      229              
  Lines       19200    19199       -1     
  Branches     2720     2720              
==========================================
- Hits         8711     8710       -1     
  Misses       9975     9975              
  Partials      514      514              

@ndevenish ndevenish merged commit 8824453 into master Oct 14, 2020
@ndevenish ndevenish deleted the cbf-compression-dxtbx branch October 14, 2020 16:21
ndevenish added a commit that referenced this pull request Oct 15, 2020
)"

Caused downstream build errors in dials.

This reverts commit 8824453.
ndevenish added a commit that referenced this pull request Oct 15, 2020
…dxtbx (#228)""

This reinstates the reverted change for building and fixing.

This reverts commit 92d379d.
Anthchirp pushed a commit that referenced this pull request Nov 13, 2020
Move CBF compression and decompression routines back into dxtbx once more
rehash of #228
ndevenish pushed a commit that referenced this pull request Feb 22, 2021
The byte-offset compression introduced by #238, #228 was incorrect, causing
problems for anything _writing_ cbf files... so dlsnsx2cbf (and probably
dials.merge_cbf).

- 0xf777 -> 0x7fff as one greater than -0x8000 - values with too large a
  negative value were being incorrectly encoded
- treatment of the fall through / last case in compression for similarity with
  decompression layout
- more thorough testing in particular tests with negative delta of magnitude
  greater than 0x8000
- test added which verifies that the HDF5 and CBF representation of the data
  from dlsnxs2cbf are the same
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.

3 participants