Updated python CARFAC to receive input in Pascals#34
Updated python CARFAC to receive input in Pascals#34JasonMH17 wants to merge 5 commits intogoogle:masterfrom
Conversation
…added appropriate change to JAX carfac_test.py
|
@dicklyon This now incorporates changes so that input scaling can be specified in parameters but defaults to input in Pascals. |
dicklyon
left a comment
There was a problem hiding this comment.
Some comments on what I think would make the code more clear for future readers, etc.
python/src/carfac/jax/carfac.py
Outdated
| class EarHypers: | ||
| """Hyperparameters (tagged as static in `jax.jit`) of 1 ear.""" | ||
|
|
||
| input_scale: int |
There was a problem hiding this comment.
Why is this an int? Float/double would make more sense.
There was a problem hiding this comment.
This has been replaced by float
python/src/carfac/jax/carfac.py
Outdated
| """All the parameters set manually for designing CARFAC.""" | ||
|
|
||
| fs: float = 22050.0 | ||
| input_scale: int = 94 |
There was a problem hiding this comment.
Here is the hint that it's in dB SPL. Better hint would be in the name, eg "input_scale_dbspl".
There was a problem hiding this comment.
input_scale -> input_scale_dbspl throughout Numpy and JAX carfac.py
python/src/carfac/jax/carfac.py
Outdated
| ) | ||
|
|
||
| def __init__(self, fs=22050.0, n_ears=1, use_delay_buffer=False): | ||
| def __init__(self, fs=22050.0, input_scale=94, n_ears=1, use_delay_buffer=False): |
There was a problem hiding this comment.
I'm not sure how this works in Python, but if there's a positional parameter option, and there might be calls to this with n_ears in that position, then this would break them, no? Better to put the new parameter last?
There was a problem hiding this comment.
I haven't implemented as of yet as typically whenever there are changes from the default code, I have seen the keyword being used. equally I have made input_scale_dbspl the second parameter in CarfacDesignParameters because, like fs, it is independent of n_ears and therefore sits outside the vast majority of params in "ear_params". Happy to change if you think this might still be an issue.
python/src/carfac/jax/carfac.py
Outdated
|
|
||
| Args: | ||
| fs: Samples per second. | ||
| input_scale: scale for input waves. By default, input is expected in Pascals i.e. 94 dB SPL @ RMS=1, |
There was a problem hiding this comment.
This still doesn't say that the parameter is in dB SPL with default 94. And what does "CARFAC is considered as 107 dB SPL @ RMS=1" mean. Probably it means "while the original v1 and v2 CARFAC used an input scale of 107 dB SPL (for RMS = 1)".
There was a problem hiding this comment.
Changed as suggested
python/src/carfac/jax/carfac.py
Outdated
|
|
||
| Args: | ||
| input_waves: the audio input. | ||
| input_waves: the audio input in Pascals. |
There was a problem hiding this comment.
"in Pascals" seems wrong. Maybe "in Pascals with default input_scale". Or is this now after the scaling has been accounted for. It's hard for me to follow in the diffs, which don't show me enough context, and if I try to see more context the file way too big and I get lost. I'm not very good at this github thing.
There was a problem hiding this comment.
Changed as suggested...as you indicate the current changed means that unless explicitly specified with "input_scale_dbspl" the expected input audio should be in pascals.
python/src/carfac/jax/carfac_test.py
Outdated
| n_ears = 1 | ||
| random_generator = jax.random.PRNGKey(random_seed) | ||
| run_seg_input = jax.random.normal(random_generator, (n_samp, n_ears)) | ||
| run_seg_input = jax.random.normal(random_generator, (n_samp, n_ears)) * 10 ** ((107-94)/20) |
There was a problem hiding this comment.
It's a good thing you're in Python 3. Many languages, including C and Python 2, would evaluate (107-94)/20 as 13/20 -> 0. Include a decimate point or several to make the point more clear, in case someone ports it.
There was a problem hiding this comment.
I actually removed this as this existed prior to "input_scale_dbspl" parameter...and it is probably best to use that instead
There was a problem hiding this comment.
Especially to confirm that calling this parameter does what it says on the tin.
python/src/carfac/jax/carfac_test.py
Outdated
| n_samp = 200 | ||
| random_generator = jax.random.PRNGKey(random_seed) | ||
| run_seg_input = jax.random.normal(random_generator, (n_samp, n_ears)) | ||
| run_seg_input = jax.random.normal(random_generator, (n_samp, n_ears)) * 10 ** ((107-94)/20) |
There was a problem hiding this comment.
Why do we want to run this test at 107 dB SPL? Just for compatibility with old test? Better say so if that's it. Did we run all the various tests at this high level? Should we fix?
There was a problem hiding this comment.
These tests date from CARFAC v2 therefore all inputs were at native CARFAC scaling of 107 dB SPL for RMS =1. As noted above, I have changed "input_scale_dbspl" to account for the changes to carfac.py so that the test data matches golden data. As for the absolute level, I have left it for what I presume is RMS=1. Happy to bring the level down and generate new golden data if desired!!
python/src/carfac/np/carfac.py
Outdated
| the input_waves are assumed to be sampled at the same rate as the | ||
| CARFAC is designed for; a resampling may be needed before calling this. | ||
|
|
||
| input_waves are considered as being in Pascals and therefore should equal |
There was a problem hiding this comment.
instead of "should equal", say "their level is"
There was a problem hiding this comment.
changed as suggested
python/src/carfac/jax/carfac.py
Outdated
|
|
||
| Args: | ||
| fs: Samples per second. | ||
| input_scale: scale for input waves. By default, input is expected in Pascals i.e. 94 dB SPL @ RMS=1, |
There was a problem hiding this comment.
The correct SI unit name is "pascals" with lowercase P. No SI units are capitalized, except in abbreviations, e.g. P.
There was a problem hiding this comment.
Changed as indicated
|
|
||
| fs = 22050.0 | ||
| t = np.arange(0, 1, 1 / fs) # A second of noise. | ||
| amplitude = 1e-3 # -70 dBFS, around 30 or 40 dB SPL |
There was a problem hiding this comment.
It might be better to stay compatible by changing the test waveform amplitude here, rather than first assuming 107 and then converting to 94? Or leave a hint about how we got here.
There was a problem hiding this comment.
Since we now use the CARFAC v1/v2 input scaling, this can remain the same I guess?!
…ffectively. Also reverted to using 107 dB SPL scaling for carfac_test.py by implementing input_scale_dbspl parameter in carfac_design function
…ectively. As in Numpy carfac_test.py, input scaling is reverted to native CARFAC v1/v2 107 dB SPL. Title added for Numpy/JAX carfac_test.py files to more easily differentiate
|
@dicklyon Thanks for the comments...I have made some changes as suggested and also pivoted to testing the input_scale_dbspl parameter within CARFACDesignParameters in the test functions. |
This update adds scaling of conventional audio input in Pascals to the CARFAC-specific scaling i.e. from 94 to 107 dB SPL for stimuli with RMS equal to 1.
Additional changes are made to the test stimuli so that they are scaled in Pascals.