pyfar / pyfar

python package for acoustics research
https://pyfar.org
MIT License
73 stars 14 forks source link

type checking in `Signal.sampling_rate` #533

Open artpelling opened 7 months ago

artpelling commented 7 months ago

The sampling_rate kwarg in the Signal constructor is not thoroughly typechecked or parsed which leads to errors down the line that are hard to backtrace. IMO sampling_rate should be cast as an integer. Of course having sampling_rate as a 2d-array is not a great design choice but happens sometimes, e.g. when reading data from .mat files

MWE:

import numpy as np
import pyfar as pf

n = np.random.rand(100)
fs = np.array([[100]])
sig = pf.Signal(n, sampling_rate=fs)
pf.plot.group_delay(sig) # this fails
f-brinkmann commented 7 months ago

I think we once discussed that it should be possible to have float sampling rates to be most flexible, but it definately should not be an array. We could cast it to int, if abs(int(sampling_rate) - sampling_rate) < tolerance. Would that be a good compromise?

artpelling commented 7 months ago

Would that be a good compromise?

I was thinking more somehing like:

if hasattr(sampling_rate, '__iter__'):
    assert len(sampling_rate) == 1
    sampling_rate = sampling_rate[0]

?

f-brinkmann commented 7 months ago

Ah, yes - that would be needed but would not cast to integer. But it works woth float as desired:

signal = pf.Signal([0, 1, 0, 0], 22.5)
gd = pf.dsp.group_delay(signal)
artpelling commented 6 months ago

Regarding casting to integer: I noticed that a float fails when writing to file, i.e.

pyfar.io.write_audio(pyfar.Signal(array, sampling_rate=44100.0))

will fail. I think writing to file should round to the nearest integer, cast the type and throw a warning on nonzero roundoff. What do you think?

f-brinkmann commented 6 months ago

Can you check your pyfar version. It works for me in 0.6.3 and only thros an error if acutally having a non integer sampling rate. We fixed it a while ago following the idea you also have :)