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

Don't try to be clever about filling the output buffer with zeros #1847

Merged
merged 1 commit into from Jun 2, 2018

Conversation

Projects
None yet
6 participants
@mgeier
Contributor

mgeier commented Jun 2, 2018

@coveralls

This comment has been minimized.

coveralls commented Jun 2, 2018

Coverage Status

Coverage remained the same at 51.386% when pulling 5398b90 on mgeier:anti-buzzing into 3950332 on psychopy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 2, 2018

Codecov Report

Merging #1847 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1847   +/-   ##
=======================================
  Coverage   46.74%   46.74%           
=======================================
  Files         221      221           
  Lines       33844    33844           
  Branches     5678     5678           
=======================================
  Hits        15822    15822           
  Misses      16452    16452           
  Partials     1570     1570
Impacted Files Coverage Δ
psychopy/sound/backend_sounddevice.py 62.71% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3950332...5398b90. Read the comment docs.

@peircej peircej merged commit 7a9e739 into psychopy:master Jun 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@peircej

This comment has been minimized.

Member

peircej commented Jun 2, 2018

I didn't know about ndarray.fill() and it should be more efficient than multiplying by zero, but I'm not clear why it would cause any functional difference to the result. Any idea why that would fix this bug?

@hoechenberger

This comment has been minimized.

Member

hoechenberger commented Jun 2, 2018

@mgeier mgeier deleted the mgeier:anti-buzzing branch Jun 2, 2018

@mgeier

This comment has been minimized.

Contributor

mgeier commented Jun 2, 2018

The output array (sometimes) contains garbage bytes which turn out as NaN or +/- infinity when interpreted as float32 values. Those values don't become zero when multiplied by zero, they rather become NaNs which then end up being audible (they are probably turned into +/- 1 at some point on their way to the sound card?).

This is also the reason why the message invalid value encountered in multiply was appearing.

@peircej

This comment has been minimized.

Member

peircej commented Jun 3, 2018

Good to know. Thanks Matthias!

@hoycw

This comment has been minimized.

hoycw commented Jun 28, 2018

Thanks so much for helping fix this issue!

Sorry for a noob question, but in terms of pulling the fix, how bad an idea would it be to edit this one line in my current distribution of psychopy (v1.85.3)? I assume this fix was merged into the newest 1.90.2, but to get it via update, I'd need to work out some logistics (mainly checking if other people's experiments would break, but also getting NeuroDebian to add the update to their distribution).

@peircej

This comment has been minimized.

Member

peircej commented Jun 28, 2018

There were a number of further fixes made to the sounddevice engine recently (e.g. fixes to the hanning window for synthesized sounds) so I think I'd recommend you copy over the entire file rather than just that line. But yes, I think that could be dropped into your existing installation with no trouble, and if it does raise new problems then it's easy to undo the change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment