Add concatenate function #452

twennemann · 2023-03-29T17:42:21Z

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

Closes #

Changes proposed in this pull request:

Add a concatenation function, concatenating Signals along specified channels.
Add broadcasting parameter, which broadcasts the signals into largest cdim and a common shape, ignoring caxis.
Please rename to concatenate_channles

Otherwise approved

twennemann · 2023-04-05T11:58:53Z

I tried to adjust the comment of the merged signal so that the comments can be assigned to the data in the merged signal. First, to the comment of the merged signal is passed on which caxis the signals were merged. If there is a comment in one of the input Signals, that comment is appended, as well as the corresponding channels on caxis.
Questions:

Is it right to call it channels?
Do we start counting at 0 or at 1? For example first_signal.cshape[caxis] is 3, would we say the comment corresponds to channel 1-3 or 0-2?

f-brinkmann · 2023-04-28T10:53:35Z

pyfar/utils.py

+    Merge multiple Signal objects along a given axis. The signals are copied,
+    if needed broadcasted in largest dimenson and to a common cshape and a new
+    object is returned.
+
+    The :py:mod:`cshape <pyfar._concepts.audio_classes>` of the signals are
+    broadcasted following the `numpy broadcasting rules
+    <https://numpy.org/doc/stable/user/basics.broadcasting.html>`_


I would suggest to make this shorter and move information about the broadcasting to the Parameters section. caxis could be linked to the audio concepts, where it is explained in more detail

Suggested change

Merge multiple Signal objects along a given axis. The signals are copied,

if needed broadcasted in largest dimenson and to a common cshape and a new

object is returned.

The :py:mod:`cshape <pyfar._concepts.audio_classes>` of the signals are

broadcasted following the `numpy broadcasting rules

<https://numpy.org/doc/stable/user/basics.broadcasting.html>`_

Merge multiple Signal objects along a given caxis.

f-brinkmann · 2023-04-28T10:56:30Z

pyfar/utils.py

+        to axis (the first, by default). If this is the case, set
+        ``broadcasting=True``.
+    caxis : int
+        The axis along which the signals are concatenated.


Suggested change

The axis along which the signals are concatenated.

The caxis along which the signals are concatenated.

We should also add a link to the concepts (see above)

f-brinkmann · 2023-04-28T10:59:01Z

pyfar/utils.py

+        If this is ``True``, the signals will be broadcasted into largest
+        dimension and into a common cshape, except of the caxis along which the
+        signals are concatenated. The default is ``False``.


Suggested change

If this is ``True``, the signals will be broadcasted into largest

dimension and into a common cshape, except of the caxis along which the

signals are concatenated. The default is ``False``.

If this is ``True``, the signals will be broadcasted to common

cshape, except for the caxis along which the signals are

concatenated. The default is ``False``.

f-brinkmann · 2023-04-28T11:02:35Z

pyfar/utils.py

+    for signal in signals:
+        if not isinstance(signal, pf.Signal):
+            raise TypeError("All input data must be of type pf.Signal")


I would suggest to make the function work also for Frequency and TimeData. Maybe _assert_matching_meta_data can already be used to check if all objects are of the same type.

f-brinkmann · 2023-04-28T11:04:02Z

pyfar/utils.py

+    comment: string
+        A comment related to the merged `data`. The default is ``""``, which
+        initializes an empty string. Comments of the input signals will also
+        be returned in the new Signal object. These comments are marked with
+        their corresponding signal position in the input tuple.


I would suggest to simply the comments handling. Use an empty string if the provided comment. Discard comments from input data. What do the others think?

f-brinkmann · 2023-04-28T11:34:11Z

pyfar/utils.py

+        cshape = np.broadcast_shapes(*np.delete(cshapes, caxis,
+                                     axis=-1))
+        broad_signals = []
+        for i, s in enumerate(signals):


using

Suggested change

for i, s in enumerate(signals):

for signal, cshape in zip(signals, cshapes):

might be better to read and avoid indexing

f-brinkmann · 2023-04-28T11:35:04Z

pyfar/utils.py

+    if comment != "":
+        comment = comment + '\n'
+    comment = comment + f'Signals concatenated in caxis={caxis}.\n'
+    sig_channel = 0
+    for s in signals:
+        if s.comment != "":
+            comment += f"Channel {sig_channel+1}-"\
+                       f"{sig_channel + s.cshape[caxis]}: "\
+                       + s.comment + '\n'
+        sig_channel += s.cshape[caxis]


See above. Should we simplify this?

f-brinkmann

Thanks for implementing. Approved and awaiting a discussion on the comment handling...

f-brinkmann · 2023-05-03T06:13:39Z

pyfar/utils.py

+    signals : tuple of Signal, TimeData or FrequencyData
+        The signals to concatenate. All signals must be of the same object type
+        and either have the same cshape or be broadcastable to the same cshape,
+        except in the dimension corresponding to axis (the first, by default).


Better to use caxis instead of axis.

f-brinkmann

I just recognized that this would need small changes to work for complex signals. But I would suggest to make this a separate pull.

the complex variable must be obtained from _assert_matching_meta_data which requires check and test audio arithmetic for complex-valued data #464
before concatenating s._data the domain must be set to time if the input data contains a complex Signal object.
the complex flag must be set when returning Signal or TimeData objects

noted in #384

ahms5

Thank you for implementing, i just have a few comments on the usage und docstring

ahms5 · 2023-06-26T08:08:31Z

pyfar/utils.py

+        The default is ``False``.
+    Returns
+    -------
+    merged : Signal


Suggested change

merged : Signal

merged : Signal, TimeData or FrequencyData

ahms5 · 2023-06-26T08:13:18Z

pyfar/utils.py

+
+    Parameters
+    ----------
+    signals : tuple of Signal, TimeData or FrequencyData


should it be possible to also concatenate along the frequency axe? or is it another fucntion?

good to have, but should be another function

ahms5 · 2023-06-26T08:16:41Z

pyfar/utils.py

+    Parameters
+    ----------
+    signals : tuple of Signal, TimeData or FrequencyData
+        The signals to concatenate. All signals must be of the same object type


maybe we should use a more general name for signals, like data or audio?

Add conc function and tests

e9cb5c9

twennemann requested review from f-brinkmann, sikersten and mberz March 29, 2023 17:42

Docstring

8ee0081

f-brinkmann mentioned this pull request Mar 31, 2023

Ignore axis in broadcasting #430

Closed

twennemann added 2 commits April 5, 2023 13:40

Update conc_sig.comment

e5d6576

Update utils.py

647b6f1

Update Docstring

ed709b6

mberz mentioned this pull request Apr 14, 2023

add channel concatenation function #65

Closed

f-brinkmann requested changes Apr 28, 2023

View reviewed changes

twennemann added 2 commits May 2, 2023 17:31

Requested changes

a2cd7f1

Accept Time and FreqData + Tests

281057a

twennemann requested a review from f-brinkmann May 2, 2023 16:08

Update test_utils.py

62f47a7

f-brinkmann previously approved these changes May 3, 2023

View reviewed changes

Update utils.py

2601c01

twennemann dismissed f-brinkmann’s stale review via 2601c01 May 3, 2023 08:47

Remove comment parameter

1c9c738

f-brinkmann self-requested a review May 11, 2023 15:49

f-brinkmann requested changes May 11, 2023

View reviewed changes

f-brinkmann mentioned this pull request Jun 2, 2023

Roadmap to complex signals #384

Open

34 tasks

f-brinkmann self-requested a review June 2, 2023 11:14

f-brinkmann previously approved these changes Jun 2, 2023

View reviewed changes

ahms5 requested changes Jun 26, 2023

View reviewed changes

Rename function into concatenate_channels

d04e4d3

twennemann dismissed f-brinkmann’s stale review via d04e4d3 July 12, 2023 12:40

twennemann requested review from ahms5 and f-brinkmann July 12, 2023 12:42

f-brinkmann approved these changes Jul 12, 2023

View reviewed changes

mberz merged commit 362ddd9 into develop Jul 14, 2023
7 checks passed

mberz deleted the feature/concatenate_function branch July 14, 2023 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add concatenate function #452

Add concatenate function #452

twennemann commented Mar 29, 2023 •

edited

twennemann commented Apr 5, 2023

f-brinkmann Apr 28, 2023

f-brinkmann Apr 28, 2023

f-brinkmann Apr 28, 2023

f-brinkmann Apr 28, 2023

f-brinkmann Apr 28, 2023

f-brinkmann Apr 28, 2023

f-brinkmann Apr 28, 2023

f-brinkmann left a comment

f-brinkmann May 3, 2023

f-brinkmann left a comment •

edited

ahms5 left a comment

ahms5 Jun 26, 2023

ahms5 Jun 26, 2023

f-brinkmann Jun 30, 2023

ahms5 Jun 26, 2023

	The axis along which the signals are concatenated.
	The caxis along which the signals are concatenated.

	for i, s in enumerate(signals):
	for signal, cshape in zip(signals, cshapes):

Add concatenate function #452

Add concatenate function #452

Conversation

twennemann commented Mar 29, 2023 • edited

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

Changes proposed in this pull request:

twennemann commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-brinkmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-brinkmann left a comment • edited

Choose a reason for hiding this comment

ahms5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twennemann commented Mar 29, 2023 •

edited

f-brinkmann left a comment •

edited