COO-256 Multiple image planes in DetImage._data (data, std, count) - #20
Yashvi-Sharma wants to merge 1 commit into
Conversation
…hods to mostly have the same behavior as before so the tasks are not affected.
| raise ValueError("Cannot determine shape of DetImage from metadata or outputs.") | ||
|
|
||
| def build_full_mask(self): | ||
| def build_full_mask(self) -> bool: |
There was a problem hiding this comment.
What is the thought for having partial masks with filled with NaN instead of failing?
Could also have a downstream effect when users run show() and the np.ma.masked_array treats any non-zero value as "masked". NaN is considered non-zero, so the entire region belonging to the not-yet-processed output would treated as masked
There was a problem hiding this comment.
good catch, fill_value should be zero. It shouldn't fail though if only some outputs have a certain mask, that could be intentional behavior, for example if one output has vignetting mask (like in DEIMOS).
| self.data = xr.DataArray(data=np.zeros(dim_pix, dtype=float), coords=coords, dims=["y", "x"]) | ||
| # Initialize masks Dataset | ||
| mask_keys = self.det_images[0].masks.data_vars.keys() if self.det_images[0].build_full_mask() else [] | ||
| data_vars = list(self.det_images[0].get_data('all').data_vars) |
There was a problem hiding this comment.
This looks at the first DetImage in the list to decide which variables exist (e.g. ['data'], or ['data', 'std', 'count']). This would then assumes every other DetImage in the list has those exact same variables. What happens when we have a mixture?
There was a problem hiding this comment.
probably shouldn't ever be the case in practice, and I don't think it's a priority to handle it (by handle it I mean do something "best effort"). But I think it should be checked and fail more gracefully than an unexpected KeyError down the line.
There was a problem hiding this comment.
yes in practice it should never happen as the detimages belonging to a focal plane would probably go through all the same steps and get same data_vars. But if it still needs handling:
Option 1: I move the empty data array assignment inside the loop so any new data_vars not present already are handled. This would create a big dataset for large focal plane and highly heterogeneous mixtures but since we are assuming that is rare it shouldn't be a problem otherwise.
Option 2: I add a warning and skip any missing data_vars in the loop. 'data' data_var is required to be present in all DetImage._data datasets and checking that is already handled at DetImage creation time, so there shouldn't be a case where _data is a XR.Dataset but without any data_vars at all.
Thoughts?
| if di.data is None: | ||
| didataset = di.get_data('all') | ||
| dimasks = di.masks | ||
| if di is None: |
There was a problem hiding this comment.
Would di ever be None? Since di = self.det_images[I]
There was a problem hiding this comment.
di can't be None since you'd have already thrown an AttributeError from the line immediately above. Maybe you mean if dimasks is None or similar?
There was a problem hiding this comment.
typo...should be didataset
3 participants
Refactored DetImage _data attribute to hold a xarray.Dataset instead of array.DataArray.
Dataset stores the main array in 'data' variable, DetImage init and set_data() handle conversion from DataArray to Dataset. But if a Dataset is passed to be set it should contain 'data' variable.
all image_utils.py helpers were modified to work with both DataArray and Dataset
DetImage.data property behaves the same as before, returns 'data' variable from Dataset
DetImage.get_data() can be used to access full Dataset or a specific data_var
Output, FocalPlaneImage methods similarly refactored with extra convenience methods.
MasterCombine now returns DetImage with three image planes (data, std, count)