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

DOC:signal: Fix typo in TransferFunction #17048

Merged
merged 1 commit into from Sep 19, 2022
Merged

Conversation

kasparthommen
Copy link

Reference issue

Closes #17047

What does this implement/fix?

Fixes a few documentation erros.

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks @kasparthommen for the PR. I might be wrong but the equation seems to just refer to the parameter dt which states s. Seems to me that it's a clear case of mathematical correctness vs programming logic.

@tupui tupui added scipy.signal Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org labels Sep 19, 2022
@kasparthommen
Copy link
Author

Thanks @kasparthommen for the PR. I might be wrong but the equation seems to just refer to the parameter dt which states s. Seems to me that it's a clear case of mathematical correctness vs programming logic.

Hi @tupui , thanks for taking a look. I think you're wrong: continuous transfer functions operate on a complex variable s, whereas discrete transfer functions use the complex variable z. This has nothing to do with dt.

The 1st and 3rd case clearly use the wrong variable s on the LHS because the RHS (correctly) employs z. The 2nd case is different because both the LHS and RHS are wrong; they both use s even though the equation is a discrete transfer function.

(Note that there is an additional complication in the 2nd case as there is in fact a z variable on the righ-hand side, but it doesn't refer to the complex variable used for discrete transfer functions but to the array of zeros. Therefore, I have also renamed that RHS z to q which is standard practice.)

@tupui
Copy link
Member

tupui commented Sep 19, 2022

As I said, I am purely talking about the argument names, not the mathematical correctness. With your proposal, We would miss a definition of z whereas atm s is defined in the docstring.

@kasparthommen
Copy link
Author

kasparthommen commented Sep 19, 2022

It is indeed mathematically incorrect to call a formula involving the variable z a function of s, or H(s). This error is being corrected by my PR.

Also, the s you are referring to in the docstring dt: ... Sampling time [s] has an entirely different meaning: it is simply the shorthand notation of "seconds", the physical unit of the optional parameter dt. Note here that dt is the docstring parameter, not s. The latter has no relationship with the s in H(s) whatsoever.

@ilayn ilayn changed the title Fix https://github.com/scipy/scipy/issues/17047 DOC:signal: Fix typo in TransferFunction Sep 19, 2022
@ilayn
Copy link
Member

ilayn commented Sep 19, 2022

It looks OK, thanks @kasparthommen. These are conventions anyways so we don't need to recite the control textbooks too much.

@ilayn ilayn added this to the 1.10.0 milestone Sep 19, 2022
@ilayn ilayn merged commit 130266d into scipy:main Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Documentation error in scipy.signal
3 participants