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

WIP : python3-ization and type hinting #37

Merged
merged 11 commits into from
Feb 15, 2022

Conversation

hadware
Copy link
Contributor

@hadware hadware commented Dec 15, 2019

Same thing as for pyannote-core.

First question: for what you call "array-like", i'm proposing 3 solutions:

  • create this type and use it everywhere : ArrayLike = Union[Sequence, np.ndarray]
  • only use Sequence
  • only use np.ndarray

As for now, it doesn't look like there is anything usable for type-hinting array sizes (after a quick research).
Ref: https://mypy.readthedocs.io/en/latest/cheat_sheet_py3.html#standard-duck-types (for Sequence)

@hbredin
Copy link
Member

hbredin commented Jan 8, 2020

Happy new year @hadware !

Thanks for this pull request and sorry for the delay in looking at it.
Looks like it does not pass the tests. Can you please fix that when you find the time?

@hadware
Copy link
Contributor Author

hadware commented Jan 9, 2020

Yes I will! I'll be here on next tuesday @ens by the way.

@hadware
Copy link
Contributor Author

hadware commented Feb 12, 2022

Well, this should be close to done.

Regarding my former question: there's a nifty ArrayLike type in numpy.typing that does the job nicely.

@hadware hadware mentioned this pull request Feb 12, 2022
pyannote/metrics/base.py Outdated Show resolved Hide resolved
@hadware
Copy link
Contributor Author

hadware commented Feb 13, 2022

Fixed your comments. I'm wondering: in some cases (notably: for all detection metrics), the required type for ref/hypothesis seems to be Annotation (because it then calls uemify which "tacitly" only accepts Annotation instances). Shouldn't uemify be able to also process Timeline instances?

Moreover, maybe we could extend the use of collar and skip_overlap (used in IER) to other metrics?

@hbredin
Copy link
Member

hbredin commented Feb 14, 2022

Fixed your comments. I'm wondering: in some cases (notably: for all detection metrics), the required type for ref/hypothesis seems to be Annotation (because it then calls uemify which "tacitly" only accepts Annotation instances). Shouldn't uemify be able to also process Timeline instances?

It could be applied to Timeline instances but only do the cropping part, but not the common timeline part.
This is because, Timeline instances are mostly used for segmentation metrics and we don't want to artificially split long segments into shorter ones.

Moreover, maybe we could extend the use of collar and skip_overlap (used in IER) to other metrics?

Once again, we'd have to be careful about segmentation metrics as these two notions probably don't make sense here.

Anyway, this should not be part of this PR.

Shall I merged this PR or do you plan to add more typing?

@hadware
Copy link
Contributor Author

hadware commented Feb 14, 2022

Ok, maybe in another PR, it'll indeed take some time to look into it.

Regarding this current PR, I think i'm mostly done with typing, i'll re-check everything, and it should be good.

@hbredin
Copy link
Member

hbredin commented Feb 15, 2022

C'est votre dernier mot, Jean-Pierre ?

@hadware
Copy link
Contributor Author

hadware commented Feb 15, 2022

image

@hbredin hbredin merged commit 14af03c into pyannote:develop Feb 15, 2022
@hbredin
Copy link
Member

hbredin commented Feb 15, 2022

🎉 Thanks @hadware !

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