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

Define type alias for length-6 tuple of numbers for mesh bounds, rename color_like type #3777

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

adeak
Copy link
Member

@adeak adeak commented Jan 3, 2023

This is a quick take on trying to define a type alias instead of Tuple[float, float, float, float, float, float] added in #3747, see also some discussion in #3747 (comment), with the additional complication of allowing numpy numbers and ints here.

Some of the changes necessary for mypy to be happy might render this more trouble than it's worth, see comments below. Some design questions are also open for debate.

@adeak adeak added the maintenance Low-impact maintenance activity label Jan 3, 2023
Copy link
Member Author

@adeak adeak left a comment

Choose a reason for hiding this comment

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

Cc. @banesullivan @MatthewFlamm. Do you think this is worth keeping with the cast() calls? I'm not sure. If you think it's worth it I'll add some comments to explain the casts.

@@ -31,3 +31,4 @@
# Overwrite default docstring, as sphinx is not able to capture the docstring
# when it is put beneath the definition somehow?
color_like.__doc__ = """Any object convertible to a :class:`Color`."""
bounds_like = Tuple[Number, Number, Number, Number, Number, Number]
Copy link
Member Author

@adeak adeak Jan 3, 2023

Choose a reason for hiding this comment

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

  1. Should we rename these _likes both with CapCase to match the rest and the typing module? (color_like might be used downstream...?)
  2. Should we restrict ourselves to Union[float, np.floating] instead of Number which also allows Python and NumPy ints?

Copy link
Member

Choose a reason for hiding this comment

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

  1. While we're at it, yes. Type aliases appear to (after a brief look at https://docs.python.org/3/library/typing.html) to all be PascalCase. As you know well, I'm not stranger to expanding the scope of PRs, and I think that we should hard deprecate color_like. We can leave an alias of it color_like = ColorLike and make a note somewhere, but I doubt it's being used much externally.
  2. Let's just go with Number. Reason being, the following work:
import numpy as np
import vtk

osrc = vtk.vtkOutlineSource()
osrc.SetBounds(np.arange(6))
osrc.SetBounds((1, 2, 3, 4, 5, 6))
osrc.SetBounds((1.0, 2.0, 3.0, 4.0, 5.0, 6.0))

Therefore, bounds like is really any number.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

Copy link
Member

Choose a reason for hiding this comment

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

We currenty have Vector = Tuple[float, float, float]... should we similarly change this and other types?

Also should we use the Vector type to annotate and "point" values? e.g., center()

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. We have pyvista.color_like in the main namespace, and it's documented as such. But if I want to add a helpful deprecated alias (with a doctsring that points the user to the new name) then I'll have to pull in the old deprecated name into the main namespace.

Would it be better to let downstream crash fast and loudly when we pull out color_like?

pyvista/core/composite.py Show resolved Hide resolved
pyvista/core/composite.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #3777 (9cfdac8) into main (b2eb4e6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9cfdac8 differs from pull request most recent head e60906b. Consider uploading reports for the commit e60906b to get more accurate results

@@           Coverage Diff           @@
##             main    #3777   +/-   ##
=======================================
  Coverage   94.01%   94.01%           
=======================================
  Files          82       82           
  Lines       18566    18572    +6     
=======================================
+ Hits        17454    17460    +6     
  Misses       1112     1112           

@adeak adeak changed the title Define type alias for length-6 tuple of numbers Define type alias for length-6 tuple of numbers for mesh bounds Jan 3, 2023
@akaszynski
Copy link
Member

Putting this in the main chat so this info isn't lost:

We currenty have Vector = Tuple[float, float, float]... should we similarly change this and other types?

Depends on the scope and scale of this PR. I'd say we should work on the low hanging fruit, but limit it to a handful of types.

Also should we use the Vector type to annotate and "point" values? e.g., center()

Yep, and that can be one of the low hanging ones.

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

This is good to go. Let's leave any further type improvement for follow up PRs since we're already touching 25 files here.

Ignoring the missing coverage check because it's a codecov thing and not our own issue.

@adeak adeak added the breaking-change Changes that break backwards compatibility, especially changed default parameters. label Jan 5, 2023
@adeak
Copy link
Member Author

adeak commented Jan 5, 2023

OK, thanks @akaszynski. I've added the breaking-change label, because this removes the technically public pv.color_like name (minor breakage downstream).

@adeak adeak changed the title Define type alias for length-6 tuple of numbers for mesh bounds Define type alias for length-6 tuple of numbers for mesh bounds, rename color_like type Jan 5, 2023
@banesullivan banesullivan mentioned this pull request Jan 5, 2023
Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for cleaning this up @adeak!

@banesullivan
Copy link
Member

Merging so that these annotations can be used in other open PRs

@banesullivan banesullivan merged commit 39f7d06 into pyvista:main Jan 5, 2023
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that break backwards compatibility, especially changed default parameters. maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants