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

small fixes following #21 #26

Merged
merged 3 commits into from
Dec 20, 2023
Merged

Conversation

carlocastoldi
Copy link
Contributor

@carlocastoldi carlocastoldi commented Dec 19, 2023

This PR:

  • uses intersect_with_plane() from newest vedo instead of the backported function as there is no longer the need to duplicate code from an updated dependency thanks to Bring package up to date #21
  • fixes some vedo function calls still using lowerCamelCase instead of snake_case

Additionally I changed Plane.from_norm() typing to return a Self type instead of a vd.Plane because, well, that's what it returns. If this is a problem because typing.Self is not in previous python version, I can just remove the type hinting

@carlocastoldi carlocastoldi force-pushed the dep-update branch 2 times, most recently from 73035e1 to e0c2e32 Compare December 19, 2023 23:59
@carlocastoldi carlocastoldi changed the title Use intersect_with_plane() from newest vedo instead of the backported function small fixes to #21 Dec 20, 2023
@carlocastoldi carlocastoldi changed the title small fixes to #21 small fixes following #21 Dec 20, 2023
@adamltyson adamltyson self-requested a review December 20, 2023 11:46
@adamltyson
Copy link
Member

Hi @carlocastoldi, thanks for this PR!

We're currently supporting Python 3.9, 3.10 & 3.11, so unfortunately we can't use any functionality that's only available in newer versions. mypy is also complaining with:

brainglobe_heatmap/plane.py:1: error: Module "typing" has no attribute "Self"  [attr-defined]
brainglobe_heatmap/plane.py:1: note: Use `from typing_extensions import Self` instead
brainglobe_heatmap/plane.py:1: note: See https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-new-additions-to-the-typing-module
Found 1 error in 1 file (checked 17 source files)

Would you mind fixing (or removing) the typing, then I'll review.

Copy link

codecov bot commented Dec 20, 2023

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Member

@adamltyson adamltyson 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 @carlocastoldi!

@adamltyson adamltyson merged commit f9e7f90 into brainglobe:main Dec 20, 2023
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