Fixes #587: Improved FFT Normalization Handling in Arithmetic Operations #593

robogeisha · 2024-04-21T14:53:08Z

Which issue(s) are closed by this pull request?

Closes #587

Summary

Improvement on the _match_fft_norm function to support a wider range of arithmetic operations and introduction of a norm_override capability. Changes aim to improve the function's flexibility and its ability to handle exceptions more gracefully.

Changes proposed in this pull request:

in classes/audio.py

_assert_match_for_arithmetic

Modification to the parameter handling.
Addition of a new variable operation_type to handle operations explicitly.
Update of the fft_norm calculation to use the new operation_type variable.
Error message correction for mismatched times.

_match_fft_norm

Complete rewrite of the function to include more robust error handling and support for an operation parameter that dictates how the FFT normalization should be handled based on the operation type (multiply, divide, etc.).
Introduction of warnings for unsupported operations and mismatched norms without an override.
Dynamic determination of resulting FFT normalization based on the operation and input norms.

in pyfar/dsp.py

deconvolve

The function description and parameters are rewritten.
The fft_length parameter handling is clarified, and conditions on its usage are explicitly mentioned.
Error handling is expanded to provide more specific feedback on errors related to signal types and mismatched sampling rates.
Revised error messages and documentation on how the function handles input signals and output processing.

convolve

Introduction of a new operation parameter to manage different arithmetic operations affecting the convolution process, interfacing with _match_fft_norm.
Enhanced error handling to provide user feedback regarding mismatched sampling rates and unsupported operation modes.
Added handling for FFT normalization based on the specified operation, with new warnings to guide correct usage.

in test_audio_signal_arithmetic.py

test_signal_inversion

Before: The test directly attempted to invert a signal without any exception or warning checks.
After: The test now expects a warning when a division operation involves a signal with FFT normalization set to 'none'.

test_assert_match_for_arithmetic

The assertion was updated from expecting a normalization result of 'rms' to 'none'.
A change in the default or expected behavior of the arithmetic matching function.

test_convolve_mode_and_method

Tests are updated to ensure the convolution operation handles the mode and method parameters.

in test_dsp.py

test_convolve_mode_error

Error message specificity was increased from "Invalid mode" to "Unsupported mode 'invalid'".

in test_dsp deconvolve.py

test_fft_norm

Updated to include a more descriptive assertion message and testing for user warnings when mismatched norms without overrides are used.
Validates that the system correctly handles FFT normalization settings after complex operations like deconvolution,

test_fft_length_error

The error message was made more precise regarding the requirements for FFT length in signal processing operations.
Ensures that the system correctly enforces minimum FFT lengths.

in test_match_fft_norm.py

New tests to reflect the new behaviour.

f-brinkmann · 2024-04-21T15:22:38Z

Hi, and thanks for the Contribution! It's a hard to see, what you changed because there are many, many files touched. Can you please revoke the changes in tests/test_plot_data (we only update those, when we change something in the plot module), add pyfar_venv to .gitignore, and revoke all changes that only change the code style (e.g., changing everystring to use " instead of '). I assume the latter might have happened automatically.

robogeisha · 2024-04-21T20:59:46Z

thank you for the feedback and sorry for the confusion. I downloaded the library through pip in my virtual environment then moved the files to my fork, which was obviously a bad decision. Update coming soon.

styling and environment settings updates

robogeisha · 2024-04-23T18:41:48Z

Hello,

I reverted to previous version in my local environment, replaced the changes and force pushed.

changed everystring to use " instead of '

and added pyfar_venv to .gitignore.

maybe there are still some mistakes, are you coming to Audiotechnik class tomorrow? @f-brinkmann

f-brinkmann

Thanks for the effort, I could now see what you changed. We did not discuss this issue in the dev team yet, but I think we are looking for a different kind of solution. You relaxed the rules that we apply for arithmetics to raise a warning instead of an error of the case from #587 occurs. I assume, we rather want to adapt the checks to not neither raise an error nor a warning but still raise errors if the other rules are violated.

I'm not at Audiotechnik today, but I will be in office tomorrow and on Friday. If you want you can drop by to discuss the solution. You can call before to make sure I'm not in a meeting :)

f-brinkmann · 2024-04-24T06:09:03Z

pyfar/classes/audio.py

-    sampling_rate : number, None
-        Sampling rate of the signals. None, if no signal is contained in `data`
-    n_samples : number, None
-        Number of samples of the signals. None, if no signal is contained in
-        `data`
-    fft_norm : str, None
-        FFT norm of the first signal in `data`, if all FFT norms are None.
-        Otherwise the first FFT norm that is not None is taken.
-    times : numpy array, None
-        The times if a TimeData object was passed. None otherwise.
-    frequencies : numpy array, None
-        The frequencies if a FrequencyData object was passed. None otherwise.
-    audio_type : type, None
-        Type of the audio class if contained in data. Otherwise None.
-    cshape : tuple, None
-        Largest channel shape of the audio classes if contained in data.
-        Otherwise empty tuple.


I prefer the explicit documentation with a description of each return parameter.

f-brinkmann · 2024-04-24T06:11:48Z

pyfar/classes/audio.py

    times = None
    frequencies = None
    audio_type = type(None)
    cshape = ()

+    # determine operation type based on division flag
+    operation_type = "divide" if division else "multiply"


This is a bit misleading. The reason we check for the division is that different rules apply in this case. But the operation can be addition, subtraction or multiplication. Maybe using the division flag as before is cleaner.

f-brinkmann · 2024-04-24T06:20:58Z

pyfar/classes/audio.py

-    Helper function to determine the fft_norm resulting from an
-    arithmetic operation of two audio objects.
-
-    For addition, subtraction and multiplication:
-    Either: one signal has fft_norm ``'none'`` , the results gets the other
-    norm.
-    Or: both have the same fft_norm, the results gets the same norm.
-    Other combinations raise an error.
-
-    For division:
-    Either: the denominator (fft_norm_2) is ``'none'``, the result gets the
-    fft_norm of the numerator (fft_norm_1).
-    Or: both have the same fft_norm, the results gets the fft_norm ``'none'``.
-    Other combinations raise an error.


This actually gives more information than the new version, which might be helpfull for debugging. We should keep the old version IMO

f-brinkmann · 2024-04-24T06:22:22Z

pyfar/classes/audio.py

-        ``True`` if arithmetic operation is division.
-
+    fft_norm1 : str
+        The FFT normalization of the first signal. Must be one of 'none',


We have a convention to show input values as code in the docstrings. Thats why we for example used ``'none'``. This should be reverted.

f-brinkmann · 2024-04-24T06:24:20Z

pyfar/classes/audio.py

+    operation : str
+        The arithmetic operation to be performed. Supported operations are
+        'multiply', 'divide', 'add', or 'subtract'.


See comment above, I would be in favor of keeping the division flag and its old description.

f-brinkmann · 2024-04-24T06:26:50Z

pyfar/classes/audio.py

+    UserWarning
+        Warns about the use of an invalid norm, unsupported operations,
+        mismatched norms without override, or specific concerns
+        about operations involving 'none' normalization.


We actually want to be strict here and raise errors, not warning in case something is not as defined in our rules for arithmetics.

f-brinkmann changed the base branch from main to develop April 21, 2024 15:14

f-brinkmann changed the base branch from develop to main April 21, 2024 15:15

ahms5 changed the base branch from main to develop April 22, 2024 07:34

ahms5 changed the base branch from develop to main April 22, 2024 07:35

ahms5 and others added 3 commits April 23, 2024 20:13

add action to create issue on cookiecutter-pypackage

651d283

add label

e92ad94

styling and environment settings updates

964cd8b

styling and environment settings updates

robogeisha force-pushed the bugfix/fft-norm-operation-handling branch from f4c6ff3 to 964cd8b Compare April 23, 2024 18:22

f-brinkmann requested changes Apr 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #587: Improved FFT Normalization Handling in Arithmetic Operations #593

Fixes #587: Improved FFT Normalization Handling in Arithmetic Operations #593

robogeisha commented Apr 21, 2024 •

edited

f-brinkmann commented Apr 21, 2024

robogeisha commented Apr 21, 2024

robogeisha commented Apr 23, 2024

f-brinkmann left a comment

f-brinkmann Apr 24, 2024

f-brinkmann Apr 24, 2024

f-brinkmann Apr 24, 2024

f-brinkmann Apr 24, 2024

f-brinkmann Apr 24, 2024

f-brinkmann Apr 24, 2024

Fixes #587: Improved FFT Normalization Handling in Arithmetic Operations #593

Are you sure you want to change the base?

Fixes #587: Improved FFT Normalization Handling in Arithmetic Operations #593

Conversation

robogeisha commented Apr 21, 2024 • edited

Which issue(s) are closed by this pull request?

Summary

Changes proposed in this pull request:

f-brinkmann commented Apr 21, 2024

robogeisha commented Apr 21, 2024

robogeisha commented Apr 23, 2024

f-brinkmann left a comment

Choose a reason for hiding this comment

f-brinkmann Apr 24, 2024

Choose a reason for hiding this comment

f-brinkmann Apr 24, 2024

Choose a reason for hiding this comment

f-brinkmann Apr 24, 2024

Choose a reason for hiding this comment

f-brinkmann Apr 24, 2024

Choose a reason for hiding this comment

f-brinkmann Apr 24, 2024

Choose a reason for hiding this comment

f-brinkmann Apr 24, 2024

Choose a reason for hiding this comment

robogeisha commented Apr 21, 2024 •

edited