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

ENH: tf2zpk positive or negative powers #4872

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

endolith
Copy link
Member

Matlab has 2 functions, tf2zp for positive power transfer functions and tf2zpk for inverse powers. Instead of making 2 separate functions I added a var parameter to specify the variable. I used var={'s', 'z', 'z^-1'}, similar to Matlab's tf command. (The actual variable name is ignored and only the ^-1 part matters.) Python doesn't use z^-1 notation for exponents, so it could be changed to z**-1 instead.

It could alternatively be something like negative_powers=True.

Also added the eqtflength function which it depends on. This could be renamed instead of using Matlab's name.

Matlab's function can also output n and m, the orders of the original numerator and denominator. I don't know what this is used for, but it could be added later with an output= parameter if needed.

@FRidh
Copy link
Contributor

FRidh commented May 13, 2015

If I'm correct tf2zpk is for discrete signals, which are generally written as inverse powers, and tf2zp is for continuous signals, which are generally written as positive powers. That would mean there's only two options, right?

All that matters then is that you can distinguish between a analog and a digital signal. Therefore, I think the option analog=False (like with the filters) would be enough. When you use analog==True you would get positive powers and when using analog==False you would get inverse powers.

@endolith
Copy link
Member Author

Well it's common to write digital filters as positive powers, too, typically in control systems:

http://www.nptel.ac.in/courses/108103008/22
http://www.dii.unisi.it/~control/ctm/examples/motor2/digital.html
http://ctms.engin.umich.edu/CTMS/index.php?example=Introduction&section=ControlDigital#14

I thought writing var='z' would be more convenient to remember than something like positive_powers=True, but I'm open to other ideas.

But yes, there are only 2 options.

@argriffing
Copy link
Contributor

The CI is failing because eqtflength should be added to __all__?

@endolith
Copy link
Member Author

ok fixed that

@rgommers rgommers added the enhancement A new feature or improvement label May 17, 2015
@rgommers
Copy link
Member

Does eqtflength need to be public? What it does (appending zeros) is fairly trivial....

@rgommers
Copy link
Member

I like negative_powers=True better. And also would like a more Pythonic name for eqtflength.

@FRidh
Copy link
Contributor

FRidh commented Jun 11, 2015

If you can write digital filters with positive powers as well, then you have 3 options, like you offer for var. If you want to offer all 3 then I think having two keywords would be clearest, analog=True and negative_powers=False. Not sure what the defaults should be. In case of analog=True and negative_powers=True you could raise a ValueError.

I can't think of any good name for eqtflength but perhaps it makes sense to instead put the emphasis on the fact that this function can be used to convert from negative to positive powers. to_positive_transfer_function ?

@endolith
Copy link
Member Author

negative_powers=False or positive_powers=True? First is kind of a double negative.

Change `var` argument to `negative_powers` boolean.  Change eqtflength
to private function for now.
@endolith
Copy link
Member Author

Ok I changed _eqtflength to private for now, and changed the argument to negative_powers, though that feels a little clunky to me. Can you think of something shorter?

@endolith
Copy link
Member Author

More doctest failures, in one certain version, due to printing arrays differently. Should I remove the print statement then? A guide on the best way to write doctests would be nice.

@ev-br
Copy link
Member

ev-br commented Oct 12, 2015

The (modified) doctest checker does not understand the array-print syntax, [1. 2. 3.]. While it's possible to extend it, the easiest way ATM is to just omit the print statement and write simply
```>>> np.sort(z), np.sort(p), k`

@endolith
Copy link
Member Author

If you can write digital filters with positive powers as well, then you have 3 options, like you offer for var. If you want to offer all 3 then I think having two keywords would be clearest, analog=True and negative_powers=False.

Analog vs digital doesn't matter here. It's just finding the poles and zeros of rational functions, which is the same either way. Analog is typically written as positive powers and digital is typically written as negative, but both could be written either way.

Also this parameter should be added to all the other functions that deal with tf or sos (which is stages of tf, which I ran into here: #3717 (comment)) So if there's a shorter or more intuitive way to write it, that would be good. In some it will change the output and others it changes the interpretation of the input.

@endolith
Copy link
Member Author

Actually not necessary for tf2sos or sos2tf since we can assume input and output are both written in the same convention?

@endolith
Copy link
Member Author

endolith commented Jan 9, 2016

(and do I also need to do this for zpk2tf?)

@lucascolley lucascolley added the needs-decision Items that need further discussion before they are merged or closed label Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement needs-decision Items that need further discussion before they are merged or closed scipy.signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants