Repository: pyfar/pyfar

Issue:
array slicing bug? for non cartesian properties.
## Description
It seems to have sth to do with the conversion to cartesian coordinates.
For some reason we don't jump into the setter method.
Not sure if we can fix it easily.

## What I Did

```
import pyfar as pf
import numpy as np
c = pf.Coordinates.from_spherical_elevation([np.pi, np.pi/2], 0, 1)
c.azimuth[0] = 0
c.azimuth
```
array([3.14159265, 1.57079633])

```
import pyfar as pf
import numpy as np
c = pf.Coordinates([2, 3], 1, 1)
c.cartesian[0, 0] = 0
c.cartesian
```
array([[2., 1., 1.],
          [3., 1., 1.]])

for x,y,z it works
```
import pyfar as pf
import numpy as np
c = pf.Coordinates([1, 2, 3], 0, 0)
c.x[0] = 0
c.x
```
array([0., 2., 3.])
This just works because we return the direct reference to _x here, which also might be very dangous.

Discussion:
@sikersten: Maybe this helps: https://stackoverflow.com/questions/63214416/how-do-i-define-a-setter-for-a-list-with-an-index-or-slicing@f-brinkmann: We decided to check if the above helps or `c[0].azimuth = 0` (as in pyfar audio objects). In any case we will return a copy of x, y, and z.@f-brinkmann: We decided to return a copy of x, y, z to be consistent and describe the behaviour in the docs

Solution patch:
diff --git a/pyfar/classes/coordinates.py b/pyfar/classes/coordinates.py
index 27437215..11d71840 100644
--- a/pyfar/classes/coordinates.py
+++ b/pyfar/classes/coordinates.py
@@ -41,6 +41,17 @@ coordinate systems, e.g., the `azimuth` angle is contained in two coordinate
 systems (`spherical_colatitude` and `spherical_elevation`). The table below
 lists all coordinates.
 
+.. note::
+    All coordinates are returned as copies of the internal data. This means
+    that for example ``coordinates.x[0] = 0`` does not change
+    ``coordinates.x``. This can be done using
+
+    .. code-block:: python
+
+       new_x = coordinates.x
+       new_x[0] = 0
+       coordinates.x = new_x
+
 .. list-table::
    :widths: 25 75
    :header-rows: 1
@@ -1257,7 +1268,7 @@ class Coordinates():
         (:math:`-\infty` < x < :math:`\infty`).
         """
         self._check_empty()
-        return self._x
+        return self._x.copy()
 
     @x.setter
     def x(self, value):
@@ -1270,7 +1281,7 @@ class Coordinates():
         (:math:`-\infty` < y < :math:`\infty`).
         """
         self._check_empty()
-        return self._y
+        return self._y.copy()
 
     @y.setter
     def y(self, value):
@@ -1283,7 +1294,7 @@ class Coordinates():
         (:math:`-\infty` < z < :math:`\infty`).
         """
         self._check_empty()
-        return self._z
+        return self._z.copy()
 
     @z.setter
     def z(self, value):
diff --git a/pyfar/dsp/dsp.py b/pyfar/dsp/dsp.py
index 0aabd899..3ab8ee07 100644
--- a/pyfar/dsp/dsp.py
+++ b/pyfar/dsp/dsp.py
@@ -1292,7 +1292,8 @@ def find_impulse_response_delay(impulse_response, N=1):
             ir = impulse_response.time[ch]
             ir = np.real(ir) if mode == 'real' else np.imag(ir)
 
-            if np.max(ir) > 1e-16:
+            #Check absolute amximum, because peaks can be positive or negative.
+            if np.max(np.abs(ir)) > 1e-16:
                 # minimum phase warns if the input signal is not symmetric,
                 # which is not critical for this application
                 with warnings.catch_warnings():
@@ -1318,6 +1319,16 @@ def find_impulse_response_delay(impulse_response, N=1):
                 search_region = np.imag(
                     correlation_analytic[search_region_range])
 
+                # If this is true, it indicates that `correlation_analytic` has
+                # a negative peak, which can happen if the absolute maximum of
+                # `impulse_response` is negative. Changing the sign of the
+                # search region makes sure that the `mask` generated below
+                # works as intended. Fixing it this way is safer because it is
+                # theoretically possible that the absolute maximum of
+                # `impulse_response` is negative but
+                # `correlation_analytic[argmax].real` is positive.
+                search_region *= np.sign(correlation_analytic[argmax].real)
+
                 # mask values with a negative gradient
                 mask = np.gradient(search_region, search_region_range) > 0
 


Test patch:
diff --git a/tests/test_coordinates.py b/tests/test_coordinates.py
index 275e68cd..32d36d46 100644
--- a/tests/test_coordinates.py
+++ b/tests/test_coordinates.py
@@ -633,6 +633,19 @@ def test_converters():
     coordinates.cyl2cart(0, 0, 1)
 
 
+def test_disabled_property_slicing():
+    """Test if slicing is disabled for properties."""
+    x = [1, 1]
+    coordinates = Coordinates(x, 0, 0)
+    coordinates.x[0] = 0
+    npt.assert_equal(coordinates.x, x)
+
+    azimuth = [0, 0.3]
+    coordinates = Coordinates.from_spherical_elevation(azimuth, 0, 1)
+    coordinates.azimuth[0] = 1
+    npt.assert_equal(coordinates.azimuth, azimuth)
+
+
 @pytest.mark.parametrize(
     ("points_1", "points_2", "points_3", "actual", "expected"), [
         (1, 1, 1,                Coordinates(1, 1, -1),                 False),
diff --git a/tests/test_dsp.py b/tests/test_dsp.py
index e8a18f4d..98715054 100644
--- a/tests/test_dsp.py
+++ b/tests/test_dsp.py
@@ -764,7 +764,8 @@ def test_impulse_response_delay(is_complex):
     npt.assert_allclose(start_sample_est, start_sample, atol=1e-2)
 
 
-def test_impulse_response_delay_sinc():
+@pytest.mark.parametrize('sign', [1, -1])
+def test_impulse_response_delay_sinc(sign):
     """Test delay of a band-limited sinc function shifted by 1/2 samples."""
     sr = 44100
     n_samples = 128
@@ -774,6 +775,10 @@ def test_impulse_response_delay_sinc():
     sinc = np.sinc(samples - delay_samples)
     win = sgn.get_window('hann', n_samples, fftbins=False)
 
+    # Impulse responses with large negative peaks require special handling in
+    # `find_impulse_response_delay`. This makes tests the special case.
+    sinc *= sign
+
     ir = pf.Signal(sinc*win, sr)
     start_samples = pf.dsp.find_impulse_response_delay(ir)
     npt.assert_allclose(start_samples, delay_samples, atol=1e-3, rtol=1e-4)