remove format_class.get_mask() as it doesn't appear to be used

[rjgildea]

As far as I can tell the format_class.get_mask() method no longer appears to be called after the recent masking refactor. At least all the dials/dxtbx tests appear to pass after removing the methods. I don't know whether this is an oversight, a bug, or reflects a lack of test coverage.

[jmp1985]

Everything looks fine to me except for FormatPYunspecified.py which I think @phyy-nx should have a look at before the pull request is merged.

[jmp1985]

This code was written by Tara Michaels-Clark in 2015. It's not clear exactly to me what it is supposed to do but it looks like the first section applies an untrusted region mask and then the second bit applies a trusted range mask.

In the new C++ code, the trusted range mask will be set so that is fine; however, all the stuff between line 180 and line 200 will not be applied. @phyy-nx would it be possible to apply the tile stuff as untrusted rectangles in the panel model?

[phyy-nx]

Help me understand this pull request? These get_mask methods all seem very custom, not just for FormatPYUnspecified.

[rjgildea]

The point is that as a result of recent changes to the masking code the format_class.get_mask() method is no longer called, so the code being removed in this pull request is now dead code.

[biochem-fan]

What is the new way to define a mask in a dxtbx class then?

[jmp1985]

@biochem-fan There are multiple ways to define a mask.

1. The trusted range is automatically applied to mask pixel values
2. The untrusted rectangle mask is applied by specifying untrusted rectangles in the Panel object
3. A dynamic shadow mask object can be used to provide more complex dynamic masking. The Format class needs a method get_goniometer_shadow_masker which returns an object inheriting from GoniometerShadowMasker
4. A static mask can be attached the image set during import or by setting the imageset.external_lookup.mask

[biochem-fan]

@jmp1985 How do you deal with pixel masks in EIGER master files? Are they ignored after recent mask code changes?

The untrusted rectangle mask is applied by specifying untrusted rectangles in the Panel object

This is not very useful. Images from EIGER and SACLA contain a pixel mask as metadata. There is no point converting it back into rectangles, as they will be converted back into a pixel mask again internally.

A static mask can be attached the image set during import or by setting the imageset.external_lookup.mask

This is what I want, but cannot be set up from a dxtbx class?

[phyy-nx]

In agreement with @biochem_fan. The get_mask function of the format object was very helpful for defining format-specific masks that never change. This pull request does remove dead code but the dead code needs to be resurrected. I think losing the code is a regression. My vote is to close this pull request and create an issue that the get_mask functions from the format classes are no longer called.

…

On Wed, Jul 17, 2019 at 3:17 AM biochem_fan ***@***.***> wrote: @jmp1985 <https://github.com/jmp1985> How do you deal with pixel masks in EIGER master files? Will it be ignored after this PR? The untrusted rectangle mask is applied by specifying untrusted rectangles in the Panel object This is not very useful. Images from EIGER and SACLA contain a pixel mask as metadata. There is no point converting it back into rectangles, as they will be converted back into a pixel mask again internally. A static mask can be attached the image set during import or by setting the imageset.external_lookup.mask This is what I want, but cannot be set up from a dxtbx class? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65?email_source=notifications&email_token=ADGY5STVVOPM2XWB3JLT6PDP73WUVA5CNFSM4IDYWA72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2DXOUI#issuecomment-512194385>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADGY5STK6KEMMCQHZIR67FTP73WUVANCNFSM4IDYWA7Q> .

[rjgildea]

I think the primary issue with the get_mask method, and the reasoning behind the recent dynamic masking refactoring (#59) is that having to call back to a python get_mask() method is a performance blocker in threaded integration (dials/dials#705).

@biochem-fan are these masks static across the entire scan, or do they vary with each image? If the former, then it sounds like we need a way to define and assign a static pixel mask in the format class constructor which hopefully wouldn't be too difficult to implement. If the latter, then it sounds a bit like the ideas discussed in dials/dials#705. Hopefully @jmp1985 can comment further on this.

[biochem-fan]

are these masks static across the entire scan, or do they vary with each image?

At least for MPCCD at SACLA and EIGER at SPring-8, the masks should be static across the entire dataset. Note that MPCCD datasets are stills and have one Experiment per image.

Another workaround could be setting pixel values under the mask to something beyond the trusted range in get_raw_data.

[jmp1985]

@biochem-fan @rjgildea If the mask is static across the dataset then we can set it as a static mask in the ImageSet class. I guess we could add a get_static_mask method to Format which returns either a mask or None and then set that in the ImageSet when it is instantiated. This can be done as follows:

imageset.external_lookup.mask.data = ImageBool(mask_flex_array_or_tuple_of_flex_arrays)

[biochem-fan]

@jmp1985 This is a good idea, provided that a user-provided mask can be automatically merged with the mask from the format class. For example, dead pixels and pixels near the edges of MPCCD panels should be masked. These are specific to individual MPCCDs and provided as metadata in the HDF5 file. In addition, users might want to mask beam stop shadows.

[jmp1985]

@biochem-fan Currently, this is not the case but should be straightforward to implement.

[phyy-nx]

The get_static_mask sounds fine. My vote is to close this pull request and create and issue for converting the get_mask functions to get_static_mask functions and ensuring they are used.

…

On Tue, Jul 23, 2019 at 9:44 AM James Parkhurst ***@***.***> wrote: @biochem-fan <https://github.com/biochem-fan> Currently, this is not the case but should be straightforward to implement. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65?email_source=notifications&email_token=ADGY5STCZNMTUASDK3MF3H3QA4DKPA5CNFSM4IDYWA72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2TFA5Y#issuecomment-514216055>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADGY5STRN2INSWEUM4TUSFTQA4DKPANCNFSM4IDYWA7Q> .

[rjgildea]

Superseded by #73.