Conversation
sikersten
left a comment
There was a problem hiding this comment.
Thank you for getting this started!
To keep it simple, please create a new pull request for every new method and only contain existing methods in this pull request.
…ded comment.setter
ahms5
left a comment
There was a problem hiding this comment.
looks already very nice, thank you.
pyfar/classes/_pyfar_multichannel.py
Outdated
| @abstractmethod | ||
| def flatten(self): | ||
| """Return a copy of the object collapsed into one channel dimension.""" | ||
|
|
||
| pass | ||
| @abstractmethod | ||
| def transpose(self): | ||
| """Returns a copy of the object with channel axes transposed.""" | ||
|
|
||
| pass |
There was a problem hiding this comment.
i think flatten and transpose can also be implemented based on reshape, see _Audio class as an example
There was a problem hiding this comment.
works for flatten, not sure about transpose
There was a problem hiding this comment.
Transpose is quite extensive for _Audio and not defined for other classes (please correct me if I'm wrong), so I would leave it as an abstract method...
pyfar/classes/_pyfar_multichannel.py
Outdated
| @abstractmethod | ||
| def broadcast_cdim(self, cdim): | ||
| """ | ||
| Broadcast an copy of the object wth a certain channel dimension | ||
| (`cdim`). | ||
|
|
||
| Parameters | ||
| ---------- | ||
| cdim : int | ||
| The cdim to which the object is broadcasted. | ||
|
|
||
| Returns | ||
| ------- | ||
| object : _PyfarMultichannel | ||
| Broadcasted copy of the object. | ||
| """ | ||
|
|
||
| pass |
There was a problem hiding this comment.
other remark, based on what broadcast_cdim does, i would wonder if this function really make sence, no such numpy method exists.
inserting new dimension can also be done via e.g. reshape
There was a problem hiding this comment.
Agree! It is essentially a convenience function to prepend dimensions. I think we can leave it if properly documented.
| @abstractmethod | ||
| def cshape(self): | ||
| """ | ||
| Return channel shape. |
There was a problem hiding this comment.
I'm not sure what the docs should state here :P
There was a problem hiding this comment.
I'm not sure how to extend the description of cshape, as it returns the shape excluding one dimension for _Audio and Coordinates, and does not yet exist for Orientations and Filters. There is a cshape for TransmissionMatrix, as it inherits from FrequencyData, and there is also abcd_cshape, which excludes two dimensions from the cshape.
There was a problem hiding this comment.
|
i have just noticed, that |
pyfar/classes/_pyfar_multichannel.py
Outdated
| @abstractmethod | ||
| def flatten(self): | ||
| """Return a copy of the object collapsed into one channel dimension.""" | ||
|
|
||
| pass | ||
| @abstractmethod | ||
| def transpose(self): | ||
| """Returns a copy of the object with channel axes transposed.""" | ||
|
|
||
| pass |
There was a problem hiding this comment.
works for flatten, not sure about transpose
pyfar/classes/_pyfar_multichannel.py
Outdated
| @abstractmethod | ||
| def broadcast_cdim(self, cdim): | ||
| """ | ||
| Broadcast an copy of the object wth a certain channel dimension | ||
| (`cdim`). | ||
|
|
||
| Parameters | ||
| ---------- | ||
| cdim : int | ||
| The cdim to which the object is broadcasted. | ||
|
|
||
| Returns | ||
| ------- | ||
| object : _PyfarMultichannel | ||
| Broadcasted copy of the object. | ||
| """ | ||
|
|
||
| pass |
There was a problem hiding this comment.
Agree! It is essentially a convenience function to prepend dimensions. I think we can leave it if properly documented.
ahms5
left a comment
There was a problem hiding this comment.
I would suggest to assume that _data contains the data for defuld behaviour. And we could still add tests.
pyfar/classes/_pyfar_multichannel.py
Outdated
| def transpose(self, caxes=None): | ||
| """Returns a copy of the object with channel axes transposed.""" | ||
|
|
||
| pass |
pyfar/classes/_pyfar_multichannel.py
Outdated
| Broadcasted copy of the object. | ||
| """ | ||
|
|
||
| pass |
There was a problem hiding this comment.
if the fle jsut contains one class i would rename it to _PyfarMultichannel.py
…ultichannel, Improved docstrings
…im, added T(self) property, improved docstrings
f138bd7 to
ee2cfa7
Compare
ahms5
left a comment
There was a problem hiding this comment.
Thank you Hanna, looks almost ready imo. I just have small comments on tests
pyfar/classes/__init__.py
Outdated
| '_pyfar_multichannel', | ||
|
|
There was a problem hiding this comment.
| '_pyfar_multichannel', |
pyfar/classes/__init__.py
Outdated
| from . import filter | ||
| from . import transmission_matrix | ||
| from . import warnings | ||
| from . import _pyfar_multichannel |
There was a problem hiding this comment.
| from . import _pyfar_multichannel |
abstract classes are not part of the official pyfar api. you can still acess them via pyfar.classes._pyfar_multichannel._PyfarMultichannel
pyfar/classes/_pyfar_multichannel.py
Outdated
| from math import prod | ||
| from pyfar.classes._pyfar_base import _PyfarBase | ||
|
|
||
| class _PyfarMultichannel(_PyfarBase, ABC): |
There was a problem hiding this comment.
| class _PyfarMultichannel(_PyfarBase, ABC): | |
| class _PyfarMultichannel(_PyfarBase): |
_PyfarBase already inherit from ABC
| Broadcasted copy of the object. | ||
| """ | ||
|
|
||
| signal = self.copy() |
tests/test_pyfar_multichannel.py
Outdated
| @patch.multiple(_PyfarMultichannel, __abstractmethods__=set()) | ||
| def test_cshape(): | ||
| """Test the cshape property.""" | ||
| data = np.array([[[1, 1, 1], [1, 1, 1]], [[2, 2, 2], [2, 2, 2]]]) |
There was a problem hiding this comment.
| data = np.array([[[1, 1, 1], [1, 1, 1]], [[2, 2, 2], [2, 2, 2]]]) | |
| data = np.ones((2,2,2)) |
this would be much easier to read
tests/test_pyfar_multichannel.py
Outdated
| @pytest.mark.parametrize('cshape', [(5, 1), (2, 3)]) | ||
| def test_reshape_value_error(cshape): | ||
| """Test whether ValueError is raised for the reshape method.""" | ||
| data = np.array([[[1, 2, 3], [4, 5, 6]], [[7, 8, 9], [10, 11, 12]]]) |
tests/test_pyfar_multichannel.py
Outdated
| @patch.multiple(_PyfarMultichannel, __abstractmethods__=set()) | ||
| def test_flatten(): | ||
| """Test the flatten method.""" | ||
| data = np.array([[[1, 2, 3], [4, 5, 6]], [[7, 8, 9], [10, 11, 12]]]) |
tests/test_pyfar_multichannel.py
Outdated
| rng = np.random.default_rng() | ||
| data = rng.random((6, 2, 5, 256)) |
There was a problem hiding this comment.
| rng = np.random.default_rng() | |
| data = rng.random((6, 2, 5, 256)) | |
| data = np.arange(2*3*4).reshape((1, 2, 3, 4)) |
better not to use randomized values in tests. tests should be determistic
tests/test_pyfar_multichannel.py
Outdated
| @pytest.mark.parametrize('taxis', [(2, 0, 1), (-1, 0, -2)]) | ||
| def test_transpose_positional_arguments(taxis): | ||
| rng = np.random.default_rng() | ||
| data = rng.random((6, 2, 5, 256)) |
tests/test_pyfar_multichannel.py
Outdated
| @patch.multiple(_PyfarMultichannel, __abstractmethods__=set()) | ||
| def test_transpose_errors(): | ||
| rng = np.random.default_rng() | ||
| data = rng.random((10, 10, 256)) |
| cshape : tuple, list, np.ndarray | ||
| The channel shape to which the object is broadcasted. |
There was a problem hiding this comment.
I'm not entirely sure, is it a good idea to allow all these data types for cshape?
9c5d755 to
796276a
Compare
As proposed in the Issue #835, here is a draft implementation of the _NumPyBase class. This abstract class should define the NumPy-like functions that must then be implemented by the _Audio, Coordinates and other classes.
Edit: From the fourth commit , the class _NumPyBase is now called _PyfarMultichannel.