Fix negative real handling and improve pole matching logic#1216
Fix negative real handling and improve pole matching logic#1216UbeenII wants to merge 6 commits into
Conversation
slivingston
left a comment
There was a problem hiding this comment.
Initial comments:
- Why are the commits associated with a GitHub user different from the user who is opening this PR? (Neither user account has a real name, so authorship is unclear.)
- A change that results in
_ideal_tfinal_and_dt()returning a different tfinal is not ideal because it will change behavior of several public functions, e.g.,impule_responsewhentimeptsarg is not given. - Ideally, when fixing a bug, you have a test that reproduces it; i.e., a regression test.
b59fba9 to
d037196
Compare
…eals takes the -1 root and uses the p_nr case causes t_emp to become infinite due to ln|p|=0 causing a division by zero and a RuntimeWarning. FIX exclude |p|=1 so they use the m_w case which uses cycle counting formula which is appropriate for unit circle poles. 2nd fix: The m_int case used p.real -1 <sqrt_eps instead of np.abs (p.real-1)<sqrt_eps. Without abs any pole with Re(p) < 1 were matched rather than just poles near +1 causing stable poles like 0.95 to be discarded as discrete integrators. I updated the corresponding test to have the expected value to match the correct result. Fixes python-control#1204.
d037196 to
8821978
Compare
|
Sorry this is my first time contributing. I have fixed the dual author problem and I think GitHub now reflects the correct account now. I added an additional test to this case: |
ad376f0 to
8821978
Compare
|
I realised the previous failure on the CI was due to an accidental change I made to one of the tests ( changing 13.8 to 25) I've restored it to the original main branch value (13.81551) and verified that the entire suite passes cleanly locally now. Ready for review whenever you get a chance |
|
Thanks; I will review it tomorrow. |
|
coverage: 94.736%. remained the same — UbeenII:fix-impulse-response-negative-real-pole into python-control:main |
slivingston
left a comment
There was a problem hiding this comment.
Thanks! I will merge this after you consider my formatting comments.
Co-authored-by: Scott C. Livingston <scott@rerobots.net>
Co-authored-by: Scott C. Livingston <scott@rerobots.net>
adjusted formatting
2 fixes. 1st fix: the m_nr branch which is meant to handle negative reals takes the -1 root and uses the p_nr case causes t_emp to become infinite due to ln|p|=0 causing a division by zero and a RuntimeWarning. FIX exclude |p|=1 so they use the m_w case which uses cycle counting formula which is appropriate for unit circle poles.
2nd fix: The m_int case used p.real -1 <sqrt_eps instead of np.abs (p.real-1)<sqrt_eps. Without abs any pole with Re(p) < 1 were matched rather than just poles near +1 causing stable poles like 0.95 to be discarded as discrete integrators. I updated the corresponding test to have the expected value to match the correct result.
Relaxed the T[-1] tolerance from 0.5 *ideal_dt to ideal_dt because discrete-time vectors must land on integer multiples of dt, so T[-1] can overshoot ideal_tfinal by up to one full sample period.
Fixes #1204.