Make FormatHDF5SaclaMPCCD lazy

[dwpaley]

• Add FormatMultiImageLazy as a mixin

• Bugfix in FormatMultiImage: If we're creating an ImageSetLazy, we still
  need to get the static mask from the format class and add it to the
  imageset. [Applied separately in Apply static format masks to Lazy ImageSets #268 - ND]

Co-authored-by: Aaron Brewster [email protected]

[phyy-nx]

@biochem-fan do you have time to review this since it touches the SACLA format class?

[biochem-fan]

This looks fine to me but I don't have time to test this by building the latest code and re-processing something at the moment. Can you process lysozyme or something and make sure? The important thing is that it respects both the mask defined in the HDF5 file (e.g. detector edges) and one supplied by a user (e.g. shadow of beam stopper).

[codecov]

Codecov Report

Merging #227 (9fe3947) into master (f38162a) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
+ Coverage   46.39%   46.49%   +0.09%     
==========================================
  Files         228      228              
  Lines       18995    19012      +17     
  Branches     2749     2751       +2     
==========================================
+ Hits         8813     8839      +26     
+ Misses       9677     9667      -10     
- Partials      505      506       +1     

[ndevenish]

This looks fine, as long as you are happy this doesn't break biochem-fan's cases.

Probably needs a newsfragments/227.bugfix for the Lazy masking fix.

[ndevenish]

Okay, so a couple of extra points:

• I've raised the scope of _add_static_mask_to_iset - since it doesn't capture variables, I think it reads clearer being moved - with it inline I have to study and understand the entire function when trying to read get_imageset, to check that it isn't relying on anything in surrounding scopes.
• I've merged the changes from master so that other fixes are in, and with these changes (I tested with and without the above move) the test test_single_file_indices introduced in cb6e61b fails on

  - >           assert obj.call_count == expected_call_count
  E           AssertionError: assert 2 == 4
  

  I presume that the expected call count is supposed to change, but given that you introduced the test and this change probably you are in the best place to judge that.

[ndevenish]

And, because I want to get it in for DIALS 3.3, I've separated the bugfix parts into #268 (which will be merged once the CI tests pass).

[phyy-nx]

I've fixed the test_single_file_indices problem in fa895eb and fa895eb by explicitly testing with or without the lazy parameter. I've also ran @biochem-fan's requested test. I've ran dials.import on SACLA-MPCCD-Phase3-21528-5images.h5, which has a static mask, then I've ran dials.find spots with an without a custom mask. The static mask and the custom mask are both used. I then disabled the static mask manually to double check I got different results. So I think we are good to go on this.

[ndevenish]

I'm a little concerned with the sprinkling of iset.reader().nullify_format_instance()(#245) everywhere and that it's compensating for something broken in the implementation of the Format*Lazy classes. I'd feel happier if we understood where this problem was coming from.

That said, It only seems to be in tests that use FormatHDF5SaclaMPCCD directly and I don't know if the stuff tested in e.g. test_multi_panel_gain_map is used by anything outside of this format class.

If it's just for these cases, then I guess as you are happy it's fine.

[phyy-nx]

At the moment it's just for those cases and practically speaking only matters in tests like these where the same file is loaded in different ways. It's documented in #245 (and I don't have an idea how to fix it at present).