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

Split points into Points and BasePoints in preparation for Graph layer #6867

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

DragaDoncila
Copy link
Contributor

@DragaDoncila DragaDoncila commented Apr 24, 2024

References and relevant issues

This PR is a precursor to implementing the GraphLayer - see #5861 for a WIP on the graph layer itself.

Description

In discussions during the Graph Layer working group we found that getting the PR close to review was very challenging due to frequent merge conflicts with the Points layer, which were not trivial to resolve because that PR currently makes the split you see in this PR. We therefore decided to bring this change into its own PR.

This PR therefore splits the current Points layer functionality into a _BasePoints and Points layer. This allows the GraphLayer to reuse this code to represent its nodes.

Summary of Key Changes:

data and _points_data

Previously, the list of point coordinates lived at Points.data and self.data was used throughout Points as needed. In this PR, _BasePoints uses a new property, _points_data for all implemented methods.

Points overrides this property to return self.data. This means everything that's in the base class works via _points_data, and can be reused by Graph, but Points still keeps its public API and data setters mostly the same.

_BasePoints.data getter, _BasePoints._points_data and _BasePoints._set_data are all abstract. The _BasePoints.data setter is fully implemented i.e. copied from Points, and Points uses it explicitly via _BasePoints.data.fset(self, data).

Other abstract methods on _BasePoints

  • _get_ndim: in Points it's remained the same
  • _make_slice_request_internal: in Points it's remained the same
  • add: in Points, it's remained the same. In Graph, it would still be used to add nodes but it also takes an indices argument for the nodes - so it's actually not overriding the _BasePoints add... Should we just remove add from _BasePoints entirely?
  • remove_selected: unchanged in Points, but I actually think we could extract some of the property updating stuff to _BasePoints maybe?
  • _move_points: _move (which calls _move points once it's worked out which points need moving and to where) is implemented in _BasePoints and then Points._move_points simply does the shift on the data using the given indices.

Extracted from Points to _BasePoints

  • _on_selection
  • all features stuff
  • style and properties stuff (border_color, face_color, text, symbol, size, etc.)
  • data extent/out of slice/canvas_size properties/setters
  • get_state/get_value/get_status
  • selected_data stuff - but self._selected_data is updated directly in points _paste_data
  • thumbnail stuff
  • slice stuff outside of _make_slice_request_internal

Remains in Points only

...

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 97.52747% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 92.44%. Comparing base (5527240) to head (551c7c0).
Report is 3 commits behind head on main.

Files Patch % Lines
napari/layers/points/_base.py 97.47% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6867      +/-   ##
==========================================
+ Coverage   92.43%   92.44%   +0.01%     
==========================================
  Files         614      618       +4     
  Lines       54903    55201     +298     
==========================================
+ Hits        50748    51031     +283     
- Misses       4155     4170      +15     

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

@Czaki
Copy link
Collaborator

Czaki commented May 1, 2024

Please do not forget to include similar test for a new class.

def test_docstring():
validate_all_params_in_docstring(Points)
validate_kwargs_sorted(Points)
validate_docstring_parent_class_consistency(Points)

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.

None yet

3 participants