Feature/parselmouth fix #85
Conversation
There was a problem hiding this comment.
So my top-line point is that I would object to merging this for a while, and I think we should move it from a 2.1 milestone to 2.2.
The big issue is around deprecation. This is quite a big change to the code-base, and might have unforseen effects on workflows. For example, we technically support ESPS as an alternative to praat. I don't know who uses that (if anyone), but this patch removes that support silently. That's not good for stability. Additionally, if we want to convince the DARLA team to update more frequently (and report upstream) we should make some guarantees to downstream in terms of notification and stability. So with that in mind, I think we should use this as an opportunity to develop and implement a deprecation policy.
I recommend reading MediaWiki's deprecation policy. At the very least, for this patch, I recommend we provide deprecation warnings for these in 2.1, and spend the 2.2 dev cycle hammering this out further.
Which gets to my second point: code quality. For example, a lot of praat.py extends parselmouth classes. We should be using subclasses instead of the interface developed here, but that will take a fair bit more work. We should also write a lot more tests. Migrating to parselmouth could effect results and analysis for users, so we want to make very sure that the core features are equivalent. We should have some feature tests to show that the results don't change. That's a lot to write, so delaying until 2.2 gives us that time.
Despite all the above, I think this is a very good patch, and don't want it to seem like I don't appreciate it. It's just that this has turned out to be quite a big change, and we should hammer out a roadmap to make sure we avoid unintended problems.
| @@ -62,6 +62,7 @@ | |||
| """ | |||
|
|
|||
|
|
|||
| from curses.ascii import SO | |||
There was a problem hiding this comment.
This is causing build failures on Windows machines. Not sure why.
There was a problem hiding this comment.
This is bad, but, I'm not 100% sure why this is here.
There was a problem hiding this comment.
Hmm seems to be a library for working with ascii, and we're importing the "shift out" character which denotes the start of an alternate character set (docs). It doesn't seem like we're using it, so if you don't know what it's for, we may as well remove it and save ourselves from figuring out the test failures.
| print(len(times)) | ||
| #print("ERROR! Vowel %s in word %s is too short to be measured with selected value for smoothing parameter." % (phone.label, word.transcription)) |
There was a problem hiding this comment.
Should use the logging module instead of print
fave/praat.py
Outdated
| class Formant: | ||
|
|
||
| """represents a formant contour as a series of frames""" | ||
|
|
||
| def __init__(self, name=None): | ||
| def __init__(self, name=None, formant = None, maxFormant = None): | ||
| if formant and maxFormant: | ||
| if isinstance(formant, parselmouth.Formant): | ||
| self.__times = list(formant.ts()) # list of measurement times (frames) | ||
| self.__intensities = [] | ||
| # list of intensities (maximum intensity in each frame) | ||
| self.__formants = [[formant.get_value_at_time(formant_number = N, time = T) | ||
| for N in (1,2,3) ] | ||
| for T in formant.ts()] | ||
| # list of formants frequencies (F1-F3, for each frame) | ||
| self.__bandwidths = [[formant.get_bandwidth_at_time(formant_number = N, time = T) | ||
| for N in (1,2,3)] | ||
| for T in formant.ts()] | ||
| # list of bandwidths (for each formant F1-F3, for each frame) | ||
| # !!! CHANGED: all above lists no longer include frames with only | ||
| # a minimum of 2 formant measurements | ||
| # !!! | ||
| self.__xmin = formant.xmin # start time (in seconds) | ||
| self.__xmax = formant.xmax # end time (in seconds) | ||
| self.__nx = formant.nx # number of frames | ||
| self.__dx = formant.dx # time step = frame duration (in seconds) | ||
| self.__x1 = formant.x1 # start time of first frame (in seconds) | ||
| self.__maxFormants = maxFormant # maximum number of formants in a frame |
There was a problem hiding this comment.
We should turn this into a subclass of parselmouth.Formant instead of interfacing like this.
See https://stackoverflow.com/questions/15526858/how-to-extend-a-class-in-python for an example of how to do this.
There was a problem hiding this comment.
This could be rolled out into a separate module, like the praat.Formant and esps.Formant modules are. I'd have to really think about how to make it a real subclass of parselmouth.Formant, since the formant class is written in C++.
There was a problem hiding this comment.
Yeah, I'm realizing that subclassing isn't going to work well given how the formant class comes to us---we don't create it, rather, it's returned from a function call. I started building a bridge in the most recent commits. See af8f871
|
All good points. Re: speech software deprecation, I'm not sure we need to be worried about dropping esps support since, as far as I can tell, it hasn't been actually acquirable software for about a decade or more (all links to it 404). But, I think retaining |
Eh, I guess I can get behind removal. HTK's also been moribund for a while which is where my hesitation comes from, but that's at least kinda available---ESPS isn't. As long as we're super clear about the change in the release notes, I think you're right that this is low risk.
I've added some deprecation warnings in 83cbde6 to that effect. Could be specified more (and probably include a
Hmm, that feature wasn't clear to me from the PR. I agree that's a priority and should be included in 2.1. I think the combo of deprecation, esps removal, and parselmouth inclusion is a good one. The subclassing and such can be documented as technical debt and fixed later. |
|
I haven't really messed around with the Github Project kanban boards since I set them up, but could be useful for planning a little bit of this out. https://github.com/users/JoFrhwld/projects/1/views/1 What I've managed to figure out about the history of why ESPS was included at all is from here: http://www.phon.ox.ac.uk/releases Specifically
Stephan Isard was a co-author with Keelan Evanini and Mark Liberman on the first FAVE-like system in 2008, and Ubuntu 12.04 was initially released in 2012, all immediately prior to the first verisons of the FAVE-suite being created. |
| self.lines = lines | ||
| try: | ||
| float(lines[0].split('\t')[2]) | ||
| except ValueError: | ||
| # Log a warning about having detected a header row | ||
| self.logger.warning('Header row was detected') | ||
| del lines[0] | ||
| self.lines = lines |
There was a problem hiding this comment.
This reverts the header detection from #65, we should remember to fix this before merging
Creates the new parselmouth_bridge module and deprecates some of the fave.praat module in favor of these parselmouth-based classes.
No description provided.