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

Update dsp.average for complex signals #459

Merged
merged 4 commits into from Oct 15, 2023

Conversation

tluebeck
Copy link

Raise value error for complex Signals for modes: 'log_magnitude_zerophase', 'magnitude_zerophase', 'magnitude_phase', 'power'

tluebeck added 2 commits April 28, 2023 09:22
Raise value error for complex Signals for modes: 'log_magnitude_zerophase', 'magnitude_zerophase', 'magnitude_phase', 'power'
@f-brinkmann f-brinkmann changed the title Update input checks, and implement tests Update dsp.average for complex signals May 3, 2023
@f-brinkmann
Copy link
Member

What is the idea behind not having these modes for complex signals - wouldn't the operations be possible in general?

@f-brinkmann f-brinkmann self-requested a review May 11, 2023 15:47
Copy link
Member

@f-brinkmann f-brinkmann left a comment

Choose a reason for hiding this comment

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

Thanks for implementing. It looks fine to me.

Copy link
Member

@ahms5 ahms5 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing, looks fine to me.

Copy link
Member

@ahms5 ahms5 left a comment

Choose a reason for hiding this comment

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

Thnaks again, One questions comes to my mind after the review.

'power',)):

raise ValueError((
f"mode '{mode}' is not defined for complex signals."))
Copy link
Member

Choose a reason for hiding this comment

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

what does not defined mean? according to the description above it should be defined, or does it mean it's not implemented?

Copy link
Member

Choose a reason for hiding this comment

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

If I remember our discussion correctly, we did not check in depth if there is a usefull definition and leave it up to people who need that to implement in case there is one.

@tluebeck tluebeck changed the base branch from complex_time_signals to develop_complex_signals July 28, 2023 13:30
@tluebeck tluebeck merged commit cca38ad into develop_complex_signals Oct 15, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants