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

Add button to automatically apply RGB presets #2558

Merged
merged 11 commits into from
Nov 14, 2023

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Nov 9, 2023

Pretty self explanatory, I hope. Ready for everyone to try to break it. Demo with 6 layers (note that I changed the button text and decided to set stretch to 99% for all layers since I made this recording):

Screen.Recording.2023-11-09.at.1.31.03.PM.mov

@rosteen rosteen added feature Feature request UI/UX😍 plugin Label for plugins common to multiple configurations labels Nov 9, 2023
@rosteen rosteen added this to the 3.8 milestone Nov 9, 2023
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (dc09c8e) 90.83% compared to head (bb9e0a8) 90.79%.
Report is 1 commits behind head on main.

❗ Current head bb9e0a8 differs from pull request most recent head f8a99b8. Consider uploading reports for the commit f8a99b8 to get more accurate results

Files Patch % Lines
...nfigs/default/plugins/plot_options/plot_options.py 93.10% 2 Missing ⚠️
...lt/plugins/plot_options/tests/test_plot_options.py 94.11% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2558      +/-   ##
==========================================
- Coverage   90.83%   90.79%   -0.04%     
==========================================
  Files         160      160              
  Lines       19291    19354      +63     
==========================================
+ Hits        17523    17573      +50     
- Misses       1768     1781      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rosteen
Copy link
Collaborator Author

rosteen commented Nov 9, 2023

I'll add a test tomorrow, FWIW.

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Seems to work quite well and gives us a good starting place to test against a bunch of different scenarios and tweak the underlying algorithm/defaults. I'm a bit torn about whether this is too slow that we should set the underlying glue state directly or so fast that we should intentionally slow it down so the user can see the steps being applied 😂 so I guess that means we should just leave it as-is, at least for now.

This could definitely use test coverage before merge as well, though!

@pllim

This comment was marked as resolved.

@pllim
Copy link
Contributor

pllim commented Nov 10, 2023

OK looks like expected now, I think. 96.96% of diff hit (target 90.00%)

Comment on lines 639 to 643
if n_visible > 2:
default_opacity = 1 / math.log2(n_visible)
# Sample along a colormap if we have too many layers
if n_visible > 5:
cmap = matplotlib.colormaps['gist_rainbow'].resampled(n_visible)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried about two different if-else statements that need to be synced to each other (if we ever change the length of the supported number of presets, we need to update the list above, this if-else, and the if-else in the for loop. Maybe we can create the list of colors before the for-loop and then just access and set them within the for loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I don't love all the conditionals here either. Creating a matplotlib colormap is cheap enough I think I'll just do it anytime there are more than 2 visible layers in that first if statement. Actually, instead of hard-coding numbers, I can check against the length of preset_colors.

Copy link
Member

Choose a reason for hiding this comment

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

what if we had (before the for-loop):

if n_visible > len(preset_colors):
    preset_colors = [matplotlib.colors.to_hex(cmap(i), keep_alpha=True) for i in range(n_visible)]

and then in the for-loop, just access preset_colors[i] (or better yet, loop over preset colors as well)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't quite just loop over the preset colors, since we also need to get the right index from the layer list in the loop. I made the other changes though.

Copy link
Member

Choose a reason for hiding this comment

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

Could loop over both with zip (for visible_layer, preset_color in zip(visible_layers, preset_colors)) if we don't need to access much else, but this works too

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

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

LGTM!

@rosteen rosteen merged commit cfe7e25 into spacetelescope:main Nov 14, 2023
17 checks passed
@camipacifici
Copy link
Contributor

camipacifici commented Nov 14, 2023

Sorry, coming here late. Can you please invert the color scheme? An intuitive workflow would be:

  • I load images in order of wavelength (F115W, F150W, F200W)
  • These go from low wavelength to high wavelength
  • So the colors should go from blue to red.

Currently it goes from red to blue.
Thank you!

@pllim
Copy link
Contributor

pllim commented Nov 14, 2023

Since this PR is merged, do we need a new ticket?

@rosteen
Copy link
Collaborator Author

rosteen commented Nov 14, 2023

Since this PR is merged, do we need a new ticket?

Nah, that's fast enough I'll just do it without one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request plugin Label for plugins common to multiple configurations Ready for final review UI/UX😍
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants