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

Fix bugs relating to empty GroupLayers #38

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

willGraham01
Copy link
Contributor

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Currently there are a number of issues when working with empty GroupLayer objects:

  • [BUG] Seg-fault when adding an empty group to an empty group #12
  • Adding a Layer via drag-and-drop to an empty GroupLayer when there are other empty GroupLayers in the tree causes the Layer to be added to the first empty GroupLayer, not the selected one.
  • We hit sporadic recursion errors when attempting to put GroupLayers inside other GroupLayers.

The above issues are caused by the comparison operation == on GroupLayer: this is inheriting from a napari object (TypedMutableSequence) which compares the ._list attributes of the GroupLayer. Since empty lists are equal, thus it is that empty GroupLayers are equal. In particular, this means that asking the GroupLayer.index method for the index of an empty GroupLayer, the index of the first such GroupLayer would be returned and not necessarily the GroupLayer we were concerned with!

  • This caused [BUG] Seg-fault when adding an empty group to an empty group #12 since it meant that a GroupLayer was to be placed inside itself (moving an empty GroupLayer inside a GroupLayer resulted in moving the first empty GroupLayer inside itself as references were incorrect).
  • Likewise, we hit occasional recursion errors because the selection propagation would continually propagate a selection to an object with a reference to itself.
  • And dragging-and-dropping Layers always got added to the first empty group (if placed into any such empty group) because the index method would point to the first empty group in the list.

What does this PR do?

Implements an explicit __eq__ method for the GroupLayer class, which fixes the issue of comparison. Since the GroupLayerControls need to use the GroupLayers as keys, they must also be hashable, so we also need an appropriate __hash__ method. Use of id(self) is not sufficient because the way napari works when moving objects is to delete them and instantiate a copy, thus changing the memory address of said object and hence id(self) is not invariant!

As a result, GroupLayer (the class) now tracks a __next_uid int which serves as our hash value for a given instance. When an instance is created, the uid property is set to the next available value, and then the next available value increments by 1. __hash__ can then fetch the uid attribute which has all required properties of a hash, and __eq__ can now be overwritten.

This does mean that we have the following (somewhat unintuitive) behaviour for GroupLayers now: two GroupLayer instances, instantiated with the same inputs, will not be equal (by neither == or is).

However, I would argue this is actually desirable from our POV. GroupLayers are an organisational tool, so two instances that contain the same nested structure should not be considered equal. The user may want to have one visible and the other invisible, for example. Or one translated relative to the other, with a different level of saturation / contrast, etc.

References

Closes #12. It also fixes the (unreported & aforementioned) bug @alessandrofelder found concerning moving Layers into empty GroupLayers.

How has this PR been tested?

Is this a breaking change?

Does this PR require an update to the documentation?

We can cross off one "breaking issue" from the section we have in the README 😄

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@willGraham01 willGraham01 linked an issue Jul 8, 2024 that may be closed by this pull request
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.66%. Comparing base (cf74dec) to head (df41d9c).

Additional details and impacted files
@@                Coverage Diff                 @@
##           ARC-dev-branch      #38      +/-   ##
==================================================
+ Coverage           89.36%   89.66%   +0.30%     
==================================================
  Files                   8        8              
  Lines                 470      484      +14     
==================================================
+ Hits                  420      434      +14     
  Misses                 50       50              

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

@willGraham01 willGraham01 requested a review from a team July 8, 2024 13:59
@alessandrofelder alessandrofelder merged commit 901e4a4 into ARC-dev-branch Jul 8, 2024
16 checks passed
@alessandrofelder alessandrofelder deleted the wgraham/empty-group-layers branch July 8, 2024 14:12
alessandrofelder added a commit that referenced this pull request Jul 10, 2024
* Pull sample blobs code

* Strip away everything except the open-able widget

* Some basic structures that track the existing layers

* BROKEN: Image layers don't play nice but I did combine the emitter objects

* HACK: Napari will now load up, but this hasn't really fixed the naming issue. I've just hacked in a missing attribute to all of the GroupLayer children - they need to have a Node mixin to behave properly

* I can see when layers are added, and have some kind of list. Still get SEG-faults or index errors when moving groups within groups, particularly empty group into empty group.

* Recognises add/remove layers from viewer and auto-adds to the group layers view. Still seg-fault bug when nesting two empty group layers

* Rename classes to be closer to development language

* Rename and move items to break things up

* Link up group layer selection with layer tab

* Reliable testing framework

* Fix our failing test suite (#13)

* Edit file name to be specific to our use case

* Rename single test

* Add checks on napari `main` (#18)

* Add workflow step to run tests on head of napari

* Add optional dependency on napari-latest which fetches napari from the git repo directly

* Create an optional install that uses HEAD of napari, add it to list of tests

* Add note that napari-latest install is available to README

* Update README.md

Co-authored-by: Alessandro Felder <[email protected]>

---------

Co-authored-by: Alessandro Felder <[email protected]>

* Add conftest and some fixtures for testing (#15)

* Add conftest.py for shared fixtures and do a quick model check for our QtGroupLayerModel

* Typehints don't carry through fixtures

* Allow renaming of layers / group layers (#19)

* allow renaming of groups and layers

* keep prefix for group layers and update setRoot

* remove unused tree_model

* add initial tests for renaming

* add test for editing state on double click

* move tests into test_group_layer_widget

* Correctly Subclass our `GroupLayer` (#20)

* Ensure GroupLayer subclasses from our own subclass of Node

* Define ambiguously inherited attributes (IE ensure I don't break KM's tests)

* Actually use the nested structure fixture so ruff doesn't get angry

* Reorganise the test suite fixture layout since it's starting to get bloated

* Basic tests to check is_group and strict typing

* Self-review

* Allow _check_if_already_tracking to be optionally recursive. Write test for method.

* Update src/napari_experimental/group_layer.py

Co-authored-by: Kimberly Meechan <[email protected]>

* Dammit linter

---------

Co-authored-by: Kimberly Meechan <[email protected]>

* Adds independent layer controls to widget (#22)

* add group layer controls

* fix controls for latest changes to group layer

* remove syncing of selection

* add tests for group layer controls

* fix TypeError

* expand docstrings

* react to sub-groups

* add overview and todo sections to readme (#31)

* Sync GroupLayer and Main Viewer layer orders (#26)

* Allow group_layer to return a flat index

* Enforce layer order updates through the GroupLayer events

* Rework how items are added to GroupLayers to make things more explicit

* Remove unused function

* Docstring Tidy (#27)

* Some docstrings for _widget

* Docstrings for GroupLayerNode class

* docstrings for group_layer_qt

* Docstrings for group_layer.py

* Add attributes and methods to classes where necessary

* Missed a rename

* Add tests for widget activity

* Test for layer deletion sync

* Ruff for 3.9 disagrees with 3.11

* strict keyword not in Py3.9

* Update test_and_deploy.yml (#32) (#33)

* Fix Errors when Moving Layers (#28)

* Allow group_layer to return a flat index

* Enforce layer order updates through the GroupLayer events

* Rework how items are added to GroupLayers to make things more explicit

* Remove unused function

* Docstring Tidy (#27)

* Some docstrings for _widget

* Docstrings for GroupLayerNode class

* docstrings for group_layer_qt

* Docstrings for group_layer.py

* Add attributes and methods to classes where necessary

* Missed a rename

* Write method that fixes the min failing example

* Hey look, a docstring!

* Remove double-counting of positions after destination

* Test revise index method, remove redundant variable passed

* Maybe this is correct if I have a flat index sort a priori

* Fix ordering issue and allow for Groups to be assigned a flatindex

* Add 2nd test to check moving items out of different subgroups still tracks order correctly

* Remove un-necessary variable

* Remove commented-out experimental code

* Apply suggestions from @K-Meech code review

* Add `Implementation` section to README (#34)

* MDLint + alessandrofelder -> brainglobe in README

* Create implementation deatils section

* Group and Node subclassing explanations

* Notes on indexing conventions

* Update README.md

Co-authored-by: Kimberly Meechan <[email protected]>

---------

Co-authored-by: Kimberly Meechan <[email protected]>

* Add right click menu and styling (#29)

* add group layer delegate

* rename group layer delegate

* add context for right click menu

* working minimal right click menu

* fix syncing of selection

* update docstrings and remove unused functions

* fix double click edit

* simplify right click actions and context to fix tests

* update docstrings

* Force widget in tests to have a parent (#35)

* Allows toggling visibility of group layers (#30)

* toggle visiblity for group layers

* add tests for visiblity

* update docstrings

* fix failing tests

* remove logger and thumbnail role

* fix view controls when switching from group to layer

---------

Co-authored-by: Will Graham <[email protected]>

* `main` and `ARC-dev-branch` merge commit reconcile (#37)

* Fix bugs relating to empty `GroupLayers` (#38)

* Fix seg-faults and broken drag-and-drop via overwriting index method

* Switch to unique ID trackers over reimplementing method. Allows safe overwride of __eq__ and __hash__

* Remove one bug from the README broken features section

---------

Co-authored-by: Alessandro Felder <[email protected]>

* Add docs build and populate content (#39)

* Basic Sphinx build and docs structure

* Workflow to publish docs on pushes to main

* Separate sphinx requirements from package requirements

* Add building the docs section

* Reorganise group_layers docs

* Docs API for Groups & Nodes, some source docstring updates for text rendering

* Placeholder for widget docs

* Write widget docs page

* Add note on hashing things

* expand docs for group layers, delegates and context menu (#40)

---------

Co-authored-by: Kimberly Meechan <[email protected]>

---------

Co-authored-by: Alessandro Felder <[email protected]>
Co-authored-by: Kimberly Meechan <[email protected]>
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.

[BUG] Seg-fault when adding an empty group to an empty group
2 participants