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: add note about different ba forms in tf2zpk #3307

Merged
merged 1 commit into from
Feb 24, 2014

Conversation

endolith
Copy link
Member

of course someone should confirm that what I wrote is actually true...

Feel free to suggest different wordings.

For comparison, matlab has 2 different functions: tf2zp for s and positive z, and tf2zpk for z^-1. Matlab also has the tf function, which has a {'s', 'z', 'z^-1'} parameter. A parameter like that should probably be added to scipy's tf2zpk function? I think that's more usable than creating a separate tf2zp function (do mostly the same thing, hard to remember which is which). But for now I am just documenting the discrepancy.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 92c22cd on endolith:patch-3 into * on scipy:master*.

@rgommers
Copy link
Member

@Eric89GXL would you have time to check this in the next day or so? Would be nice to have this in for 0.14.0, and there's a lot of stuff left to do....

@larsoner
Copy link
Member

Sure, I'll have a look.

@@ -244,7 +244,7 @@ def freqz(b, a=1, worN=None, whole=0, plot=None):


def tf2zpk(b, a):
"""Return zero, pole, gain (z,p,k) representation from a numerator,
r"""Return zero, pole, gain (z,p,k) representation from a numerator,
Copy link
Member

Choose a reason for hiding this comment

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

I'm just curious, why do we need the r here?

Copy link
Member

Choose a reason for hiding this comment

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

Because there are \s in the docstring, and with 'r' (which means "raw string") those get interpreted as special characters.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, so just less needing to do \\. Makes sense, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, http://legacy.python.org/dev/peps/pep-0257/ says

For consistency, always use """triple double quotes""" around docstrings. Use r"""raw triple double quotes""" if you use any backslashes in your docstrings.

but there was some controversy last time I did it? some script overwrites it or something? but then that got merged anyway so I don't know.

Copy link
Member

Choose a reason for hiding this comment

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

Not AFAIK, I just wanted to know so I could pick up that tip

Copy link
Member

Choose a reason for hiding this comment

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

Oh you're saying that there was some controversy, not asking if there was... I wonder if the docstring code gets angry or something.

Copy link
Member

Choose a reason for hiding this comment

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

The docstring machinery won't complain. There's a preference to have as little LaTeX as possible and keep docstrings readable in a terminal, maybe it was about that. In this case it's still understandable though (at least for me), so I don't really mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh here it is: #498 (comment)

guess it doesn't matter though, because that was merged and then pv recommended doing it later: #2777 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, shouldn't matter much. The doc editor became much less important over time, and anyway could be fixed if needed.

@larsoner
Copy link
Member

I looked through it, and LGTM.

@endolith
Copy link
Member Author

actually I should have added examples too, but if you want to merge it I can do those in a different PR

@rgommers
Copy link
Member

Let's merge this PR as is, then we're sure it goes into 0.14.0. Examples of course welcome.

rgommers added a commit that referenced this pull request Feb 24, 2014
DOC: add note about different ba forms in tf2zpk
@rgommers rgommers merged commit 21d55d8 into scipy:master Feb 24, 2014
@rgommers
Copy link
Member

Thanks both!

@rgommers rgommers added this to the 0.14.0 milestone Feb 24, 2014
@endolith endolith deleted the patch-3 branch February 24, 2014 15:07
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.

None yet

5 participants