mean_stdv ignores None element in list (extractFormants.py) #84
Conversation
|
I had to change a bit in the extractFormants file to check for None in the list items. |
|
Hi @DerMoehre, Joe and I are both traveling for a conference this week so we will be a bit slow to respond to PRs. One of us will review this as soon as possible, but likely not until the weekend. Thanks for the submission and patience! |
fave/extractFormants.py
Outdated
| if n > 0: | ||
| if n == 1: | ||
| mean = valuelist[0] | ||
| stdv = 0 | ||
| else: | ||
| sum_i = 0 | ||
| for i in range(n): | ||
| sum_i += valuelist[i] | ||
| mean = sum_i / n | ||
| if valuelist[i] == None: |
There was a problem hiding this comment.
This is great, but actually I think we're safe to go ahead and replace this custom mean and standard deviation code with np.nanmean() and np.nanstd(). I think what needs to change inside mean_stdv() is
- Conversion of
valuelistto annp.array() - Replacement of these loops and conditionals with
np.nanmean()andnp.nanstd().
mean = np.nanmean(valuearray)
stdv = np.nanmean(valuearray)Both the np functions return np.float64 values, which as far as I can tell will be correctly utilized by the rest of the code.
There was a problem hiding this comment.
If you're happy to make those changes as additional commits, they should appear in this PR, and we can merge them in to also include the update to the tests as well
There was a problem hiding this comment.
I will make those changes and push it to this PR :)
|
I changed the mean_stdv() to use numpy. |
|
Nice! At first glance this looks really really good. Will review and merge later today. |
|
Thank you :-) you Guys are making it really easy to get more and more into this |
There was a problem hiding this comment.
Looks good! Just a note that len(array), array.shape and array.size all have similar but slightly different behaviors for testing the length of an np array which could be useful for other error catching, but I don't think they have any performance differences so I think we'll just go ahead a merge this solution with len()
No description provided.