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

Collect and unify color overlays #473

Merged
merged 10 commits into from
Jul 2, 2024
Merged

Collect and unify color overlays #473

merged 10 commits into from
Jul 2, 2024

Conversation

talonchandler
Copy link
Collaborator

@talonchandler talonchandler commented Jul 2, 2024

Paired with https://github.com/czbiohub-sf/shrimPy/pull/141, which will close without merging.

This PR merges @edyoshikun's color overlays from shrimpy into recOrder while making necessary changes to both sets of functions:

  • accept czyx-shaped arrays as a single input for compatibility with our parallelization strategy. This required a fairly significant change to recOrder's handling of dask arrays.
  • return (3, Z, Y, X)-shaped arrays
  • do not have a channel_order parameter.

The new function signatures are:
det ret_ori_overlay(czyx, ret_max: Union[float, Literal["auto"]] = 10, cmap: Literal["JCh", "HSV"] = "JCh"):
def ret_ori_phase_overlay(czyx, max_val_V: float = 1.0, max_val_S: float = 1.0):

I have tested the new ret_ori_overlay on the example datasets (to check the lazy-computed dask arrays), and I've tested both new functions in CI.

Notes:

  • @edyoshikun, earlier I thought that you'd implemented ret_ori_phase_overlay with JCh, but I now think that this case is covered by ret_ori_overlay with the JCh option.
  • I have used this PR to clean up many of the unused pieces of colormapping code across recOrder. All of the visualization now lives in recOrder.io.visualization, and I anticipate this code will move to iphub.

Copy link
Contributor

@edyoshikun edyoshikun left a comment

Choose a reason for hiding this comment

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

this looks great! This merges the functions as we discusses and tees us up to for the porting to iphub and easier generation of these visualizations.

recOrder/io/visualization.py Outdated Show resolved Hide resolved
recOrder/io/visualization.py Outdated Show resolved Hide resolved
rescale_intensity(
retardance,
in_range=(
np.min(retardance),
Copy link
Contributor

Choose a reason for hiding this comment

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

I originally had set it up to do np.min(retardance), but maybe the right thing to have here is to clip with (0,max(retardance)). What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the current behavior is an okay start. I think iterating on good default will require testing across a wide range of datasets. I'd like to do this, but I'd vote to keep the scope of this PR tighter.

@talonchandler talonchandler requested a review from edyoshikun July 2, 2024 18:53
@talonchandler talonchandler changed the title Refactor overlay functions Collect and unify color overlays Jul 2, 2024
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 8.51064% with 43 lines in your changes missing coverage. Please review.

Project coverage is 9.54%. Comparing base (57f611e) to head (26f8a79).

Files Patch % Lines
recOrder/io/visualization.py 0.00% 41 Missing ⚠️
recOrder/plugin/main_widget.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #473      +/-   ##
========================================
+ Coverage   9.49%   9.54%   +0.04%     
========================================
  Files         29      30       +1     
  Lines       4594    4591       -3     
========================================
+ Hits         436     438       +2     
+ Misses      4158    4153       -5     

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

Copy link
Contributor

@edyoshikun edyoshikun left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for unifying the implementations @talonchandler

@talonchandler talonchandler added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit 0fa5af2 Jul 2, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants