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.

Sign up for GitHub

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

Jump to bottom

DOC: Clarify Exponent Order in ltisys.py #5650

Merged
merged 1 commit into from
Jan 8, 2016
Merged

DOC: Clarify Exponent Order in ltisys.py #5650

merged 1 commit into from
Jan 8, 2016

Conversation

gfyoung
Copy link
Contributor

@gfyoung gfyoung commented Jan 2, 2016

Title is self-explanatory.

rkern
rkern reviewed Jan 2, 2016
View reviewed changes
scipy/signal/ltisys.py
@@ -1086,6 +1096,10 @@ def lsim2(system, U=None, T=None, X0=None, **kwargs):
given to `lsim2` are passed on to `odeint`. See the documentation
for `scipy.integrate.odeint` for the full list of arguments.

If (num, den) is passed in for `system`, coefficients for both the
Copy link
Member

Choose a reason for hiding this comment

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

Either system or *system would be fine, but be consistent between the uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually was trying to match it to the argument signatures and current documentation, some of which use *system and others which use system. Should I just use system for all of them?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, it's different for this function. That's fine, then.

@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 2, 2016

@rkern : Updated with a happy Travis

@codecov-io
Copy link 

@@            master   #5650   diff @@
======================================
  Files          234     234       
  Stmts        43096   43096       
  Branches      8154    8154       
  Methods          0       0       
======================================
  Hit          33410   33410       
  Partial       2605    2605       
  Missed        7081    7081

Review entire Coverage Diff as of 389ceb7

Powered by Codecov. Updated on successful CI builds.

@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 4, 2016

Is there anything else that needs to be done here? I've just been rebasing this PR onto master (which is why I keep triggering Travis builds). Otherwise, I think this should be good to merge.

rgommers
rgommers reviewed Jan 7, 2016
View reviewed changes
scipy/signal/ltisys.py
@@ -375,6 +376,10 @@ class lti(object):
of one of its subclasses: `StateSpace`, `TransferFunction` or
`ZerosPolesGain`.

If (numerator, denominator) is passed in for `*system`, coefficients for
Copy link
Member

Choose a reason for hiding this comment

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

This needs double backticks around both *system to render correctly in html/pdf docs (single backticks is for links to other objects/functions).

Copy link
Member

Choose a reason for hiding this comment

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

same applies to changes in the rest of this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rgommers rgommers added scipy.signal Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org labels Jan 7, 2016
@rgommers
Copy link
Member

rgommers commented Jan 7, 2016

These look like good clarifications to me. Good to merge modulo the reST markup that I commented on.

@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 7, 2016

@rgommers : Should the list [1, 3, 5] be also double-backticked?

@rgommers
Copy link
Member

rgommers commented Jan 7, 2016

@rgommers : Should the list [1, 3, 5] be also double-backticked?

Less critical, but yes I have a slight preference for doing so. It's basically a trade-off between making the html output look nicer but the plain docstring a bit less readable in a terminal. s^2.. is much clearer (also because ^ is a special symbol in reST).

@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 8, 2016

@rgommers : Okay, I'll add the back-ticks to those as well.

@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 8, 2016

@rgommers : Updated with a happy Travis.

@rgommers rgommers added this to the 0.18.0 milestone Jan 8, 2016
rgommers added a commit that referenced this pull request Jan 8, 2016
@rgommers
Merge pull request #5650 from gfyoung/doc_exponent_order
0a04a46 
DOC: clarify exponent order in signal/ltisys
@rgommers rgommers merged commit 0a04a46 into scipy:master Jan 8, 2016
@rgommers
Copy link
Member

rgommers commented Jan 8, 2016

Thanks @gfyoung, merged.

@gfyoung gfyoung deleted the doc_exponent_order branch January 8, 2016 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.signal
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gfyoung @codecov-io @rgommers @rkern