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

API to load detector data with mask #518

Merged
merged 9 commits into from
Jun 4, 2024
Merged

API to load detector data with mask #518

merged 9 commits into from
Jun 4, 2024

Conversation

takluyver
Copy link
Member

@takluyver takluyver commented May 3, 2024

This continues the KeyData pattern, so you can do:

jf_det.masked_data().select_trains(np.s_[2000:2500]).ndarray()

Which is equivalent to:

mask = jf_det['data.mask'].select_trains(np.s_[2000:2500]).ndarray() != 0
imgs = jf_det['data.adc'].select_trains(np.s_[2000:2500]).ndarray()
imgs[mask] = np.nan
  • Add tests

@takluyver takluyver added the enhancement New feature or request label May 3, 2024
@@ -1102,6 +1191,12 @@

return arr


class XtdfMaskedKeyData(DetectorMaskedKeyData, XtdfImageMultimodKeyData):

Check failure

Code scanning / CodeQL

Missing call to `__init__` during object initialization Error

Class XtdfMaskedKeyData may not be initialized properly as
method XtdfImageMultimodKeyData.__init__
is not called from its
__init__ method
.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a false positive caused by CodeQL not fully understanding super() with multiple inheritance. The __init__ method is inherited from DetectorMaskedKeyData, which does call super().__init__(...). That calls the next __init__ in the method resolution order - not just the parent class - so when it's creating an XtdfMaskedKeyData, it will find XtdfImageMultimodKeyData.__init__().

In [2]: XtdfMaskedKeyData.__mro__
Out[2]: 
(extra_data.components.XtdfMaskedKeyData,
 extra_data.components.DetectorMaskedKeyData,
 extra_data.components.XtdfImageMultimodKeyData,
 extra_data.components.MultimodKeyData,
 object)

I usually avoid multiple inheritance because even if it works, it's a pain to think about stuff like this. In this instance, I started writing the masked KeyData classes as wrappers around the existing classes (has-a relationships rather than is-a), but it was going to be several times more code to provide the same methods and attributes again. So I decided using inheritance was the lesser of two evils.

@@ -831,13 +885,16 @@
)
return out

def xarray(self, *, fill_value=None, roi=(), astype=None):
def _wrap_xarray(self, arr):

Check notice

Code scanning / CodeQL

Mismatch between signature and use of an overridden method Note

Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method XtdfImageMultimodKeyData._wrap_xarray
matches the call.
Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method XtdfImageMultimodKeyData._wrap_xarray
matches the call.
@takluyver takluyver force-pushed the components-masked branch from 65219a5 to 137ed9a Compare May 3, 2024 15:13
extra_data/components.py Fixed Show fixed Hide fixed
@takluyver takluyver marked this pull request as ready for review May 3, 2024 15:39
@@ -176,6 +177,31 @@ def __init__(self, data: DataCollection, detector_name=None, modules=None,
def __getitem__(self, item):
return MultimodKeyData(self, item)

def masked_data(self, key=None, *, mask_bits=None, masked_value=np.nan):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since bit logic is not generally intuitive for pragmatic programmers, allowing to pass bad pixel bits by iterable may be nice here as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I guess we should also expose the bad pixel flags somewhere - I don't think we have those at the moment. Maybe they belong in extra.calibration ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, EXtra was the intended target for some time now. extra.calibration makes sense given it's used in calibration data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to record here, I opened European-XFEL/EXtra#172 to do that.

extra_data/components.py Fixed Show fixed Hide fixed
Copy link
Member

@tmichela tmichela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe add a link to the BadPixel values in the docstrings, and maybe a small usage example.

LGTM

The replacement value to use for masked data. By default this is NaN.
"""
key = key or self._main_data_key
self[self._mask_data_key] # Check that the mask is there
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe raise a more explicit error message here if that happens.

Copy link
Contributor

@philsmt philsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is sorely needed! LGTM.

extra_data/components.py Show resolved Hide resolved
extra_data/components.py Fixed Show fixed Hide fixed
@takluyver takluyver force-pushed the components-masked branch from 2ffe77e to 259f2c3 Compare June 3, 2024 16:47
@takluyver takluyver added this to the 1.18 milestone Jun 4, 2024
@takluyver takluyver merged commit 316b0df into master Jun 4, 2024
7 of 8 checks passed
@takluyver takluyver deleted the components-masked branch June 4, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants