Threshold masking #1544

bnmajor · 2023-07-18T22:13:05Z

Fixes #1528

psavery

Looking great so far! A few things to fix:

Toggling visibility in the mask manager dialog for the threshold mask doesn't appear to do anything
If you remove the threshold mask from the mask manager and click "Apply" in the threshold dialog, the threshold mask doesn't come back (it looks like you have to exit the program to be able to make a new threshold mask)
I think we should consider eliminating the combo boxes and just having it display two spinboxes that are a range of values that will not be thresholded. This could default to -inf to inf, and if you wanted to, for instance, threshold all values below 0 and above 50, it would be 0 to 50. But let's see if this is okay with Saransh first. I think the text is a little confusing anyways - does "Less than" mean that all values less than that value will be thresholded (masked out) or kept? Looks like it means those values should be masked out. But if I try to keep only pixels between a certain range such as 0 and 50, it doesn't look like I can do that for some reason.

hexrd/ui/mask_manager_dialog.py

hexrd/ui/image_mode_widget.py

hexrd/ui/threshold_mask_dialog.py

hexrd/ui/resources/ui/image_mode_widget.ui

hexrd/ui/threshold_mask_dialog.py

hexrd/ui/create_raw_mask.py

bnmajor · 2023-08-02T13:57:23Z

If you remove the threshold mask from the mask manager and click "Apply" in the threshold dialog, the threshold mask doesn't come back (it looks like you have to exit the program to be able to make a new threshold mask)

This seems to work for me, but please let me know if I am misunderstanding what you mean:

I think we should consider eliminating the combo boxes and just having it display two spinboxes that are a range of values that will not be thresholded. This could default to -inf to inf, and if you wanted to, for instance, threshold all values below 0 and above 50, it would be 0 to 50. But let's see if this is okay with Saransh first.

Sounds good! So something like this maybe?

My only concern is the default range - if we are defaulting to (-inf, inf) doesn't this kind of impy that everything is being masked out? Maybe that's not really an issue since that's not how it would ever be used (and we can note the behavior in a tooltip like we are now) but I'm just wondering if it may be confusing. @saransh13 Any thoughts on behavior/dialog/defaults? This simplification would eliminate support for masking values equal to a certain value but maybe that is a non-issue.

I think the text is a little confusing anyways - does "Less than" mean that all values less than that value will be thresholded (masked out) or kept? Looks like it means those values should be masked out. But if I try to keep only pixels between a certain range such as 0 and 50, it doesn't look like I can do that for some reason.

Yes, after asking Saransh we should be and-ing masks but this is confusing with the current dialog, I agree. Another point in favor of your simplified suggestion honestly 👍

psavery · 2023-08-02T14:31:28Z

If you remove the threshold mask from the mask manager and click "Apply" in the threshold dialog, the threshold mask doesn't come back (it looks like you have to exit the program to be able to make a new threshold mask)

This seems to work for me, but please let me know if I am misunderstanding what you mean:

I'll try it again tonight! (tomorrow for me)

Sounds good! So something like this maybe?

Yes that looks good!

My only concern is the default range - if we are defaulting to (-inf, inf) doesn't this kind of impy that everything is being masked out? Maybe that's not really an issue since that's not how it would ever be used (and we can note the behavior in a tooltip like we are now) but I'm just wondering if it may be confusing. @saransh13 Any thoughts on behavior/dialog/defaults? This simplification would eliminate support for masking values equal to a certain value but maybe that is a non-issue.

So, the range we are defining is the range of pixel values that will not be masked out, not the range of pixel values that will be masked out. I confirmed this with Saransh (and he thinks the range is better too).

So that means if it defaults to (-inf, inf), no pixels get masked out. Then if we change it to be (0, 50), then all pixels that do not have a value between 0 and 50 will be masked out. Does that sound good?

bnmajor · 2023-08-02T14:33:18Z

So, the range we are defining is the range of pixel values that will not be masked out, not the range of pixel values that will be masked out. I confirmed this with Saransh (and he thinks the range is better too).

So that means if it defaults to (-inf, inf), no pixels get masked out. Then if we change it to be (0, 50), then all pixels that do not have a value between 0 and 50 will be masked out. Does that sound good?

Ah, okay. This is the opposite of what I thought he was asking for. In that case this makes sense. Thanks!

bnmajor · 2023-08-02T14:34:24Z

Toggling visibility in the mask manager dialog for the threshold mask doesn't appear to do anything

This should be fixed now!

bnmajor · 2023-08-02T14:59:58Z

@psavery @saransh13 I think this is all fixed up now and ready for a final review

psavery · 2023-08-03T01:03:23Z

One more issue I see:

If I set the range to be 0 to inf, I get this error:

Traceback (most recent call last):
  File "/Users/patrick/virtualenvs/hexrd/src/hexrdgui/hexrd/ui/threshold_mask_dialog.py", line 68, in accept
    apply_threshold_mask()
  File "/Users/patrick/virtualenvs/hexrd/src/hexrdgui/hexrd/ui/create_raw_mask.py", line 21, in apply_threshold_mask
    mask = create_threshold_mask(img)
  File "/Users/patrick/virtualenvs/hexrd/src/hexrdgui/hexrd/ui/create_raw_mask.py", line 29, in create_threshold_mask
    lt_val, gt_val = HexrdConfig().threshold_values
ValueError: not enough values to unpack (expected 2, got 1)

We should also make sure setting one of the values in the range to inf or -inf works for saving/loading the mask too.

We also need to modify the cartesian and polar generation to take this masking into account, but I will look into that in a separate PR!

bnmajor · 2023-08-03T13:46:06Z

@saransh13 I tested the example that you shared with me but it seems to work for me. Can you make sure your branch is up-to-date and try again?

@psavery Thanks for that! We're now saving the range regardless of value which fixes that bug.

psavery

Let's just do the name change ("Range" -> "Unmasked Range") and then this PR looks good to me!

psavery · 2023-08-07T23:01:34Z

hexrd/ui/resources/ui/threshold_mask_dialog.ui

+   <item row="0" column="0">
+    <widget class="QLabel" name="comparison_label">
+     <property name="text">
+      <string>Range</string>


Suggested change

<string>Range</string>

<string>Unmasked Range</string>

Can we change the text to "Unmasked Range" for clarity?

psavery · 2023-08-07T23:04:46Z

hexrd/ui/mask_manager_dialog.py

@@ -250,6 +239,8 @@ def write_all_masks(self, h5py_group=None):
            for i, (det, mask) in enumerate(data):
                parent = d.setdefault(det, {})
                parent.setdefault(name, {})[str(i)] = mask
+        if self.threshold:
+            d['threshold'] = HexrdConfig()._threshold_data


I tried writing out just the masks (via "Export Masks"), and sometimes, this 'threshold' group in the HDF5 file is just an empty group. This produces an error when we load in the masks.

Maybe there are some situations where HexrdConfig()._threshold_data is invalid?

But I can't actually reproduce this issue now. Let's keep an eye out for it.

Okay, sounds good.

psavery

LGTM

Signed-off-by: Brianna Major <[email protected]>

The visibility of the threshold mask was not being saved. Makes sure that it is. Signed-off-by: Brianna Major <[email protected]>

Users are now provided two spin boxes that default to (-inf, inf). The range input will represent the range of values that will *not* be masked. Signed-off-by: Brianna Major <[email protected]>

Move threshold_mask_changed signal from threshold mask dialog to the apply_threshold_mask function. Signed-off-by: Brianna Major <[email protected]>

Signed-off-by: Brianna Major <[email protected]>

bnmajor requested review from psavery and saransh13 July 18, 2023 22:13

psavery reviewed Aug 2, 2023

View reviewed changes

hexrd/ui/create_raw_mask.py Outdated Show resolved Hide resolved

bnmajor force-pushed the threshold-masking branch from 52a366e to c19ebf0 Compare August 2, 2023 14:12

bnmajor force-pushed the threshold-masking branch from 56574ef to 1ee1fc3 Compare August 3, 2023 18:11

psavery approved these changes Aug 7, 2023

View reviewed changes

psavery approved these changes Aug 8, 2023

View reviewed changes

bnmajor added 14 commits August 8, 2023 10:51

Remove threshold masking from image tab widget

07319fc

Signed-off-by: Brianna Major <[email protected]>

Update HexrdConfig threshold mask attribute

0ae1891

Signed-off-by: Brianna Major <[email protected]>

Add threshold masking dialog

d780fa5

Signed-off-by: Brianna Major <[email protected]>

Add support for multiple conditions for threshold masks

cf0b6e8

Signed-off-by: Brianna Major <[email protected]>

Write/read threshold data to/from state files

d6f7e2c

Signed-off-by: Brianna Major <[email protected]>

Simplify check for infinity

36a97ce

Signed-off-by: Brianna Major <[email protected]>

Stack raw image mode checkboxes vertically

e5a0322

Signed-off-by: Brianna Major <[email protected]>

Remove Okay/Cancel buttons

d9f4eef

Signed-off-by: Brianna Major <[email protected]>

Fix toggle visibility for threshold masks

5cb5b67

The visibility of the threshold mask was not being saved. Makes sure that it is. Signed-off-by: Brianna Major <[email protected]>

Invert masking logic and remove combo boxes

920152c

Users are now provided two spin boxes that default to (-inf, inf). The range input will represent the range of values that will *not* be masked. Signed-off-by: Brianna Major <[email protected]>

Move threshold mask changed signal

8cf775f

Move threshold_mask_changed signal from threshold mask dialog to the apply_threshold_mask function. Signed-off-by: Brianna Major <[email protected]>

Save range, even if values are -infinity/infinity

19cb962

Signed-off-by: Brianna Major <[email protected]>

Remove unused import

f6a94a3

Signed-off-by: Brianna Major <[email protected]>

Rename: Range -> Unmasked Range

1ba89e1

Signed-off-by: Brianna Major <[email protected]>

bnmajor force-pushed the threshold-masking branch from eb4173b to 1ba89e1 Compare August 8, 2023 14:52

bnmajor merged commit e18cd7f into HEXRD:master Aug 8, 2023
9 checks passed

bnmajor deleted the threshold-masking branch August 8, 2023 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Threshold masking #1544

Threshold masking #1544

bnmajor commented Jul 18, 2023

psavery left a comment •

edited by bnmajor

Loading

bnmajor commented Aug 2, 2023

psavery commented Aug 2, 2023

bnmajor commented Aug 2, 2023

bnmajor commented Aug 2, 2023 •

edited

Loading

bnmajor commented Aug 2, 2023

psavery commented Aug 3, 2023

bnmajor commented Aug 3, 2023

psavery left a comment

psavery Aug 7, 2023

bnmajor Aug 8, 2023

psavery Aug 7, 2023

bnmajor Aug 8, 2023

psavery left a comment

Threshold masking #1544

Threshold masking #1544

Conversation

bnmajor commented Jul 18, 2023

psavery left a comment • edited by bnmajor Loading

Choose a reason for hiding this comment

bnmajor commented Aug 2, 2023

psavery commented Aug 2, 2023

bnmajor commented Aug 2, 2023

bnmajor commented Aug 2, 2023 • edited Loading

bnmajor commented Aug 2, 2023

psavery commented Aug 3, 2023

bnmajor commented Aug 3, 2023

psavery left a comment

Choose a reason for hiding this comment

psavery Aug 7, 2023

Choose a reason for hiding this comment

bnmajor Aug 8, 2023

Choose a reason for hiding this comment

psavery Aug 7, 2023

Choose a reason for hiding this comment

bnmajor Aug 8, 2023

Choose a reason for hiding this comment

psavery left a comment

Choose a reason for hiding this comment

psavery left a comment •

edited by bnmajor

Loading

bnmajor commented Aug 2, 2023 •

edited

Loading