pyannote / pyannote-metrics

A toolkit for reproducible evaluation, diagnostic, and error analysis of speaker diarization systems
http://pyannote.github.io/pyannote-metrics
MIT License
186 stars 33 forks source link

Bug with the URI parameter #28

Closed Mymoza closed 5 years ago

Mymoza commented 5 years ago

From the provided Jupyter Notebook to get started with Pyannote-metrics, it was throwing an error when running the whole notebook as is for this line:

diarizationErrorRate(reference, hypothesis, detailed=True, uem=Segment(0, 40))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-58-445b485ca05e> in <module>
----> 1 diarizationErrorRate(reference, hypothesis, detailed=True, uem=Segment(0, 40))

/srv/conda/envs/notebook/lib/python3.7/site-packages/pyannote/metrics/base.py in __call__(self, reference, hypothesis, detailed, **kwargs)
    129         else:
    130             self.uris_[uri] += 1
--> 131             uri = uri + ' #{0:d}'.format(self.uris_[uri])
    132 
    133         self.results_.append((uri, components))

TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

The uri parameter is used to remember which document it describes, it's optional. As it was empty in the Notebook, it throws an error to concatenate NoneType + str.

This PR changes the + operand for , so that the uri parameter can be None.

I tested this PR with pip install git+https://git@github.com/Mymoza/pyannote-metrics.git@mpgill-uri-bug and it works well now:

{'confusion': 7.0,
 'correct': 22.0,
 'diarization error rate': 0.5161290322580645,
 'false alarm': 7.0,
 'missed detection': 2.0,
 'total': 31.0}
hbredin commented 5 years ago

Thanks @Mymoza . I will have a closer look at this next week.

hbredin commented 5 years ago

I finally had the chance to look at your PR.

I'd rather keep uri a string rather than making it a tuple (as this might otherwise have some unexpected consequences).

Therefore, I propose we solve the issue differently. I would replace line 126 by something like

uri = reference.uri
if uri is None:
   uri = "(None)" 

... and leave the rest unchanged. Can you check that this solves the issue and update your PR accordingly?

Also please use develop branch as your base branch in the PR.

hbredin commented 5 years ago

Fixed in commit d9ca221