Skip to content
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

My contribution to the MOSQITO repository #63

Open
wants to merge 70 commits into
base: master
Choose a base branch
from

Conversation

Igarciac117
Copy link

Hi Martin,

I'm Ignacio Gracía, student of the URJC Fuenlabrada, Madrid. Here you have the result of my contribution to the project.

Best regards.

Igarciac117 and others added 26 commits July 12, 2022 13:29
@wantysal
Copy link
Collaborator

wantysal commented Aug 2, 2023

Hello Ignacio,

We are going to merge your work into a new branch to change a few things so that your code becomes coherent with the rest of the library. It will then be merged into the master branch :)

Thanks for your contribution ! We'll let you know when it's done

Copy link

@mlotinga mlotinga Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version of 61672-1 is 2013. Annex E of the standard provides an analytical expression for spectral A-weighting as a function of frequency. It would be faster, more accurate, and more straightforward, to include A-weighting as expressed in equation E.6 of 61672-1:2013 Annex E.

Additionally, and this carries more widely to any related function definitions, the 'preferred' nominal fractional octave frequency band values should not be used for signal processing purposes - the exact frequency bands should always be used, which are defined in IEC 61260-1:2014. Any calculations should be carried out using the exact frequency values and then (if required, although this is perhaps rarely necessary, except for visualisation or data storage purposes) mapped to the corresponding nominal values using a matching algorithm.

Copy link

@mlotinga mlotinga Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the code operating over a signal in a for loop? These operations can all be applied to the entire signal in a single operation without any loops. This is true even if the input comprises multiple signals, although this code doesn't seem to address that situation.

This also goes for all other functions using this loop kernel.

Moreover, if the signal is being operated on sample-by-sample (as indicated), it doesn't make sense to take a mean of a single value (line 34).

Statistical levels are typically calculated using exponentially-weighted amplitude (ie, using a time-weighting integration filter), which is then downsampled to the specified calculation interval. For example, take time signal x, apply Fast (tau=0.125 s) time-weighting RMS filter to give xFRMS, then resample down to (eg) 0.125 s intervals. Then convert to dB and take the max/min/percentile.

Even if exponential weighting is not desired, taking an RMS implies averaging over a series of samples, which this code doesn't seem to be applying.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 61672-1:2013 Annex E for analytical expression for C-weighting as a function of frequency (equation E.1). The constant C1000 needed in that equation turns out to be ~0.061904281992313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants