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

Typing of core classes #96

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

simonottenhauskenbun
Copy link

This PR should make working with Annotation easier for those using type checkers.

In my own project I neede to wrap Annotation with another class and add ugly type-checked code like this:

class Diarization:
... 
    @classmethod
    def labeled_tracks(cls, diarization: Annotation) -> List[Tuple[Segment, TrackName, Label]]:
       return list(diarization.itertracks(yield_label=True))  # type: ignore

Now typed versions of yield_label=True and yield_label=False exist. Type checkers do not allow to "set the type" by parameter value.

Details

  • fix type erros in pyannote.core.feature.py
  • fix type erros in pyannote.core.utils.types.py
  • Add named tuples SegmentTrack and SegmentTrackLabel as return types for itertracks in annotation.py

Notes on NamedTuples

SegmentTrack(NamedTuple) and SegmentTrackLabel(NamedTuple) are fully compatible with Tuple[Segment, TrackName]andTuple[Segment, TrackName, Label], i.e. they can be deconstructed via a,b,c = ...and accessed withtup[0], tup[1]In addition the fields can be accessed via dot-notation. This also works type-checked for the union type ofSegmentTrackandSegmentTrackLabel` for the common fields.

class SegmentTrack(NamedTuple):
    segment: Segment
    track: TrackName

class SegmentTrackLabel(NamedTuple):
    segment: Segment
    track: TrackName
    label: Label

s1 = SegmentTrack(...)
s2 = SegmentTrackLabel(...)

segment, track = s1 # OK!
segment, track, label = s2 # OK!

some_s: Union[SegmentTrack, SegmentTrackLabel] = s1 # or s2
print(some_s.segment) # OK!
print(some_s.track) # OK!

Copy link

@evfinkn evfinkn left a comment

Choose a reason for hiding this comment

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

Okay this is kind of crazy because I was about to make a pull request for this same reason 🤯 Thanks for doing this!


def itertracks_with_labels(self) -> Iterator[SegmentTrackLabel]:
"""Typed version of :func:`itertracks`(yield_label=True)"""
return self.itertracks_with_labels() # type: ignore
Copy link

Choose a reason for hiding this comment

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

I think this is what you meant:

Suggested change
return self.itertracks_with_labels() # type: ignore
return self.itertracks(yield_label=True) # type: ignore

However—while I'd be fine with having itertracks_with_labels and itertracks_without_labels—a better approach to getting typed itertracks might be using overload like I did here:

    @overload
    def itertracks(self, yield_label: Literal[False] = ...) -> Iterator[Track]: ...
    @overload
    def itertracks(self, yield_label: Literal[True]) -> Iterator[LabeledTrack]: ...
    @overload
    def itertracks(self, yield_label: bool) -> TrackIterator: ...

(Though this would obviously need to be slightly altered to use your NamedTuples). This has the added benefit of working with existing code. The same thing can probably be done with Timeline.crop.

Copy link
Author

@simonottenhauskenbun simonottenhauskenbun Apr 17, 2024

Choose a reason for hiding this comment

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

Thanks for the comments @evfinkn
I have tried overloaded typing with bool constants in the past, and then stumbled upon the note that overloading is only allowed for types, not values. However, I did not consider your approach of using Literal[False]. I will have to see if this works with pylance. if it does the overload approach is far more elegant 😀

edit: tested it and it works! Changed in 230cf3b

@hbredin
Copy link
Member

hbredin commented Apr 18, 2024

FYI -- a bit busy right now but I did notice this PR.

I think pyannote.core API needs some love anyway and should probably be simplified...

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.

3 participants