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.

Sign up for GitHub

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

Jump to bottom

First pass at new MaskManager class #1622

Merged
merged 27 commits into from
Jan 22, 2024
Merged

First pass at new MaskManager class #1622

merged 27 commits into from
Jan 22, 2024

Conversation

bnmajor
Copy link
Collaborator

@bnmajor bnmajor commented Nov 16, 2023

No description provided.

@pep8speaks
Copy link

pep8speaks commented Nov 16, 2023 •
edited
Loading

Hello @bnmajor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 9:1: E305 expected 2 blank lines after class or function definition, found 1

Line 88:80: E501 line too long (84 > 79 characters)

Comment last updated at 2024-01-22 18:10:05 UTC

@bnmajor bnmajor force-pushed the simplify-mask-management branch from 4cbc669 to 743c848 Compare November 16, 2023 22:32
psavery
psavery reviewed Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@psavery psavery left a comment

Choose a reason for hiding this comment

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

Overall, I think this is looking very nice! We need this refactoring improvement! Please see the suggestions, and keep up the good work! 👍

hexrdgui/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/mask_compatability.py Outdated Show resolved Hide resolved
hexrdgui/mask_compatability.py Outdated Show resolved Hide resolved
hexrdgui/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/mask_manager.py Outdated Show resolved Hide resolved
@bnmajor bnmajor force-pushed the simplify-mask-management branch 2 times, most recently from b15c9d6 to de71f6b Compare December 20, 2023 22:47
@bnmajor bnmajor changed the title WIP: First pass at new MaskManager class First pass at new MaskManager class Dec 20, 2023
@bnmajor bnmajor force-pushed the simplify-mask-management branch 3 times, most recently from da6128d to 8361673 Compare December 21, 2023 14:57
@bnmajor bnmajor force-pushed the simplify-mask-management branch from 599a956 to b886ae8 Compare January 3, 2024 14:39
@bnmajor bnmajor marked this pull request as ready for review January 3, 2024 14:40
@bnmajor bnmajor requested a review from psavery January 3, 2024 14:40
@bnmajor bnmajor force-pushed the simplify-mask-management branch from b886ae8 to b2b92d6 Compare January 3, 2024 21:04
psavery
psavery reviewed Jan 8, 2024
View reviewed changes
Copy link
Collaborator

@psavery psavery left a comment

Choose a reason for hiding this comment

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

Looking really nice so far! We really needed this re-organization! I left a few comments about ways we could improve things further.

hexrdgui/calibration/auto/powder_runner.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_compatability.py Outdated Show resolved Hide resolved
hexrdgui/calibration/polarview.py Show resolved Hide resolved
hexrdgui/masking/constants.py Show resolved Hide resolved
hexrdgui/masking/mask_manager_dialog.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
@bnmajor bnmajor force-pushed the simplify-mask-management branch from 3bec4d8 to 37b78de Compare January 8, 2024 20:31
bnmajor added 14 commits January 8, 2024 16:14
@bnmajor
First pass at new MaskManager class
6e0360b 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Fix up conversion function for old mask files
08448c0 
Keep all conversion logic in a single file and either convert to the expected
format or return the data as-is if it is already correct. Additionally add a
version key to check against and improve naming.

Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Use attributes rather than properties for now
bf80a41 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Move masking files to masking directory
1aa65ed 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Move logic from MaskManagerDialog to MaskManager
80fb656 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Finish building the MaskManager and Mask classes
21b8e6c 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Use MaskManager class instead of HexrdConfig dicts/lists
b5939b3 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Move mask signals to MaskManager
b6c51a0 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Move masking functions to MaskManager class
d7bedc0 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Remove debug statement
0d0191a 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Fix import/export of overlay masks
c3fbe46 
The powder, laue, and pinhole masks consist of a list of numpy arrays and
cannot be directly serialized/deserialized in this format. Store in a dict
instead.

Signed-off-by: Brianna Major <[email protected]>
@bnmajor
PEP8 formatting fixes
b399a73 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Make deserialize a class method
dc355b3 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Keep image dict management in HexrdConfig
d7394a3 
Move the raw_masks_dict and masked_images_dict properties and
create_masked_images_dict function back to HexrdConfig so that image dict
management is kept within HexrdConfig and the MaskManager only handles storing
masking information and computing masks.

Signed-off-by: Brianna Major <[email protected]>
bnmajor added 8 commits January 8, 2024 16:15
@bnmajor
Simplify mask state conversion logic
82e595a 
Only call the appropriate conversion function if the data needs to be converted

Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Remove unused signal
1894b4e 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Convert get_data/set_data functions to data property/setter
888783e 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Rename for clarity: setup_table -> update_table
6fc846a 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Do not keep old mask name as attribute
8796612 
Instead grab it from the mask manager after change.

Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Fix typo: mask_compatability -> mask_compatibility
1257281 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Do not reset threshold data when toggling visibility
a7973d3 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor
Save masks by detector name under the "data" key
ff03ccd 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor bnmajor force-pushed the simplify-mask-management branch from 24a6a98 to ff03ccd Compare January 8, 2024 21:16
psavery
psavery reviewed Jan 19, 2024
View reviewed changes
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
hexrdgui/masking/mask_manager.py Outdated Show resolved Hide resolved
@psavery
Copy link
Collaborator

psavery commented Jan 19, 2024

Looks like there are still a bunch of calls for mask.update_masked_arrays() in places. I think we should be able to remove them all (in some places we need to replace them with invalidate_masked_arrays()), since we now only compute the masked arrays right before they are needed.

@bnmajor bnmajor force-pushed the simplify-mask-management branch from 14cd0b1 to 852760b Compare January 21, 2024 17:20
@bnmajor
Lazily compute masked arrays
8ec3332 
Signed-off-by: Brianna Major <[email protected]>
@bnmajor bnmajor force-pushed the simplify-mask-management branch from 852760b to 8ec3332 Compare January 21, 2024 17:38
@psavery
Set version as attribute instead of dataset
9fba4bf 
I think it better belongs as an attribute, and it is easier to read
in this way.

Signed-off-by: Patrick Avery <[email protected]>
@psavery
Only invalidate masks after setting data
16cb70c 
Don't reset them just yet.

This fixes an issue with loading state files, where an exception would
be raised because the images were not loaded yet.

Signed-off-by: Patrick Avery <[email protected]>
@psavery
A few more fixes and minor refactoring...
9745a19 
Signed-off-by: Patrick Avery <[email protected]>
psavery
psavery approved these changes Jan 22, 2024
View reviewed changes
Copy link
Collaborator

@psavery psavery left a comment

Choose a reason for hiding this comment

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

I made a few small fixes, and tested nearly everything. It's working well! Great job, @bnmajor! Please look over my changes, and if they look good, then this is ready to merge!

@bnmajor
Copy link
Collaborator Author

bnmajor commented Jan 22, 2024

I made a few small fixes, and tested nearly everything. It's working well! Great job, @bnmajor! Please look over my changes, and if they look good, then this is ready to merge!

Looks, great, thank you so much!

@bnmajor bnmajor merged commit 351f24d into HEXRD:master Jan 22, 2024
9 checks passed
@bnmajor bnmajor deleted the simplify-mask-management branch January 22, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psavery psavery psavery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bnmajor @pep8speaks @psavery