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 new FOV dot visualization method #490

Merged
merged 15 commits into from
Jan 8, 2022
Merged

Add new FOV dot visualization method #490

merged 15 commits into from
Jan 8, 2022

Conversation

alex-l-kong
Copy link
Contributor

What is the purpose of this PR?

Previously, we highlighted the names of each FOV on top of their corresponding circle for the interactive remapping visualization. This is problematic because:

  1. Long FOV names may need to be displayed outside of the circle
  2. If circles overlap, the FOV names will display on top of each other and become unreadable

With this in mind, we remove the text on each FOV and instead use a green circle to highlight which are selected. This works because FOVs follow a specific numbering scheme such that it's easy to resolve which one names which.

How did you implement your changes

On the initial draw, change the way the circles are colored so that the highlighted pair is green.

On the dropdown-menu updates, make a similar change.

@alex-l-kong alex-l-kong self-assigned this Jan 4, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@alex-l-kong
Copy link
Contributor Author

Addressed:

  1. Make interactive figure display size smaller
  2. Decrease the size of the dots indicating each tile centroid
  3. Change the color of the highlighted FOV pair mapping to dark red and dark blue

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

The notebook doesn't run, remap_and_reorder_tiles fails

@alex-l-kong
Copy link
Contributor Author

alex-l-kong commented Jan 6, 2022

@ngreenwald did you run it after you clicked Save mapping? The way the notebook works, the interactive visualization won't be able to stop any cells below it from running (especially if the user runs all cells at once), which means some file paths may not exist yet (ex. mapping_path).

I don't think ipywidgets allows for this, so I specify in the comments for the interactive visualization to ignore any error messages in the final section (under Use mapping to rename...) before the user saves their mapping.

We might need to split the notebook up into different parts for the most streamlined experience. However, because all the last section does is renaming, I was hesitant to make that trivial action a separate notebook.

@ngreenwald
Copy link
Member

Yup, this was after saving

image

@alex-l-kong
Copy link
Contributor Author

@ngreenwald OK I see, I assumed that every JSON tile file would contain an id, name, and status field. I can add a check to ensure that they're only added if they exist.

@alex-l-kong
Copy link
Contributor Author

@ngreenwald can you see if the new patch works?

@ngreenwald
Copy link
Member

Yup, it works now

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

First two comments about naming apply to the entire branch, the rest are pretty minor

ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good for previous changes, I'll leave comments on main PR

@ngreenwald ngreenwald merged commit aeadd65 into tile_viz Jan 8, 2022
@ngreenwald ngreenwald deleted the viz_patch branch January 8, 2022 02:40
ngreenwald added a commit that referenced this pull request Jan 14, 2022
* Initial commit of tile visualization

* Add interactive remapping visualization draft

* Add button to save remapping

* Encapsulate logic in callback functions and fix save message error

* Suppress mplDeprecation in notebook because RTD complains about it not being a class in tiling_utils

* Remove commented-out tma = False

* Add better type checking for reading in tiling parameters

* Add some testing and a draw_radius argument for circle viz

* Commit everything but the official remapping for now

* Some bug fixes, basic tests, notebook sweeteners, etc.

* Remove the import sys paths

* Add randomization and Moly point insertion procedure after remapping

* Fix TMA from upper-right/bottom-left to upper-left/bottom-right

* Fix documentation for interactive visualization

* Change y-coordinate axis so that it goes from bottom to top ascending

* Update FOV naming conventions

* Add documentation to clarify use of the Save mapping button

* Add new FOV dot visualization method (#490)

* Update the dot size, color scheme to green on hit, and slide size

* Shrink and adjust colors for tile centroids, shrink figure size further

* Change interactive viz notebook comment to reflect new coloring scheme

* When remapping only add the metadata fields if the user specified them

* Remove import sys

* Address renaming conventions and optional randomize/Moly point settings

* Docs and PYCODESTYLE

* More documentation

* Nuke more proposed to manual

* Documentation nitpicks and official tested notebook

* Need to mock ipywidgets for documentation

* More nitpicks

* Nuke more tiles and change auto-generated FOV naming conventions to 1-indexed

* Add tested notebook

* Notebook documentation

* Address code review comments related to TMAs; add more complete testing for both TMAs and non-TMAs

* Fix documentation

* More notebook documentation

* Try another doc fix

* Fix definition of TMA in doc

* More nitpicky documentation stuff

* More documentation fixes

* Remove import sys

* Documentation

* Remapping TMA documentation

* Nuke moly_run, change to moly_region

* More notebook documentation

* Move tma setting to top of example_fov_grid_generate.ipynb

* Capitalize fov in tiling_utils

* Improve error messages

* Nuke more runs to regions

* Change tma back to true in example_fov_grid_generate.ipynb

* Doc fix

* Documentation clarification

* For test_read_tiling_param, add a test for input of the wrong type

* More run to region conversions in the test file

* My OCD is acting up

* Remove duplicate files

* Fix another comment

* Do not need to copy metadata keys over

* Split into non-TMA and TMA-specific tiling notebooks (with remapping done in the latter too); changed non_tma to tiled_region in code

* Doc fixes

* Abstract metadata key-value copying out

* Documentation

* Fix TMA/non-TMA

* Address some code review comments

* Fix more tests

* Address final code review comments

* Remove redundant FOV name to centroid dict

* update names

Co-authored-by: ngreenwald <noahfgreenwald@gmail.com>
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