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

graph layer #5861

Open
wants to merge 148 commits into
base: main
Choose a base branch
from
Open

graph layer #5861

wants to merge 148 commits into from

Conversation

JoOkuma
Copy link
Contributor

@JoOkuma JoOkuma commented May 22, 2023 •
edited
Loading

Description

This PR implements a graph layer for napari using the napari-graph.
Currently, it only supports operations at the node level (e.g., insertion, movement, removal) inherited from the points layer.

This should solve multiple open issues:

Simple example using nuclei centroids:

napari-graph-nuclei-demo.mp4

Example of performance with 1 million nodes over four dimensions with out-of-slice display:

napari-graph-1mi-nodes.mp4

TODO Before Merge

  • Update docs:

    Before doing this, I would like others' opinions over renaming edge_colors and other edge_ related attributes to node_contour_ (or something else, open to feedback) to avoid confusion with edge from the graph. Currently, it has the same nomenclature as the points layer.

  • Update napari-graph readme and provide examples on its repo.

  • Add napari-graph to pypi and conda

  • Decide if napari-graph will be an optional dependency, it depends on numba, which imposes some constraints on numpy version, @Czaki suggestion was to make numba optional to napari-graph, which could work, but it would be super slow because it uses a lot of for loops.

References

The issues mentioned above and napari-graph.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this been tested?

  • add new tests achieving high coverage of new files
  • executed all tests locally.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

cc @andy-sweet @jni @jwindhager

@github-actions github-actions bot added qt Relates to qt tests Something related to our tests labels May 22, 2023
@psobolewskiPhD psobolewskiPhD added feature New feature or request highlight PR that should be mentioned in next release notes labels May 22, 2023
@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone May 22, 2023
@kephale
Copy link
Contributor

kephale commented May 24, 2023 •
edited
Loading

I definitely support renaming edges that was one of @DragaDoncila's concerns. [edit: suggested "border" in the 2023-05-24 community meeting, this should probably be reflected in the Points api too for consistent semantics]

Making numba optional for napari-graph is certainly an effective solution, but my concern is that it might be hard for people to discover that numba is needed in the situation where they use a library that depends on napari-graph but does not depend on napari-graph itself.

@kephale
Copy link
Contributor

kephale commented May 24, 2023 •
edited
Loading

@Czaki suggests the possibility of adding a button to create an empty graph (because you can interactively build a graph)

Also suggested, since napari-graph will be an optional dependency UI and keybindings will need some logic to handle cases where napari-graph is not available in the environment.

@jni
Copy link
Member

jni commented May 26, 2023

@kephale

Making numba optional for napari-graph is certainly an effective solution, but my concern is that it might be hard for people to discover that numba is needed in the situation where they use a library that depends on napari-graph but does not depend on napari-graph itself.

Warnings are pretty noisy in napari thanks to that CPython catch_warnings bug? So we could just warn and I reckon users would catch on pretty quick. =) We could also include numba in [all]. For me that's a better option than not depending on napari-graph.

Czaki added a commit that referenced this pull request Feb 26, 2024
@jamesyan-git @JoOkuma
@DragaDoncila @pre-commit-ci @brisvag @psobolewskiPhD @Czaki
Rename edge_* attributes and references to border_* on points lay…
94dd780 
…er (#6402)

# References and relevant issues
This is a follow up to #6288 and predecessor to #5861 and was originally
discussed at the hackathon (cc @DragaDoncila, @jni).

# Description
This PR migrates all `edge_*` attributes and references on points layer
to use `border_*`, so that there is no confusion between graph edges and
other lines depicting the border of an object. Even if #5861 changes in
terms of implementation etc., **any** graph layer we implement will
likely require this change, so it's being split out from #5861 (also to
aid in review).

## TODO:
Please see [this issue
](#6541 a discussion
regarding refactoring other layer types (shapes, vectors,..)

---------

Co-authored-by: Jordao Bragantini <jordao.bragantini@czbiohub.org>
Co-authored-by: Jordão Bragantini <jordao.bragantini@gmail.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>
Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
@kephale
Copy link
Contributor

kephale commented Feb 29, 2024

Can we verify this very related issue while closing this PR, please: #2260

@kephale
Copy link
Contributor

kephale commented Feb 29, 2024

@cmalinmayor, i ran into this old issue. it is kinda related to the edge highlighting we're working on: #2560 and @quantumjot would probably be happy to see something like this for edges in cell tracking graphs

@github-actions github-actions bot added design Design discussion preferences Issues relating to the creation of new preference fields/panels maintenance PR with maintance changes, labels Mar 26, 2024
DragaDoncila
DragaDoncila reviewed Apr 24, 2024
View reviewed changes
napari/_vispy/layers/points.py
@@ -136,6 +137,9 @@ def _on_highlight_change(self):
pos = self.layer._highlight_box
width = scaled_highlight

# FIXME: vispy bug? LineVisual error when going from 2d to 3d (or the opposite)
Copy link
Contributor

@DragaDoncila DragaDoncila Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@JoOkuma I'm going through to split up points changes into their own PR - is this line part of a bad merge or did you add this? If you added it, can you expand a bit more what the bug is? Maybe this should be its own tiny PR

@DragaDoncila DragaDoncila mentioned this pull request Apr 24, 2024
Draft
@jni jni modified the milestones: 0.5.0, 0.5 May 31, 2024
@kephale kephale mentioned this pull request Jun 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jni jni jni requested changes

@andy-sweet andy-sweet andy-sweet left review comments

@Czaki Czaki Czaki requested changes

@DragaDoncila DragaDoncila DragaDoncila left review comments

@jamesyan-git jamesyan-git jamesyan-git left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@JoOkuma JoOkuma

Labels
design Design discussion feature New feature or request highlight PR that should be mentioned in next release notes maintenance PR with maintance changes, preferences Issues relating to the creation of new preference fields/panels qt Relates to qt tests Something related to our tests
Projects
None yet
Milestone
0.5
Development

Successfully merging this pull request may close these issues.

None yet