-
Notifications
You must be signed in to change notification settings - Fork 439
Add support for continuous delay systems #1148
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
base: main
Are you sure you want to change the base?
Conversation
…m to solve delay ODE in timeresp. Add litlle tests about timereps
…n the DDE integration
implemented only basic dde solving based on Skogestad python
…improve doc and julia notebook. Clean the code
…alize with json file. Fix scalar multiplication
…ests. TODO: clean dde file, test functions in it, and reloacate dde response tests. More tests also like mimo force response are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial round of comments. I haven't yet checked functionality, but found a few style issues that should be updated.
control/dde.py
Outdated
U = _check_convert_array( | ||
U, legal_shapes, "Parameter `U`: ", squeeze=False, transpose=transpose | ||
) | ||
print("U shape: ", U.shape) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a debugging statement that was left in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in last commit
control/dde.py
Outdated
discontinuity_times.remove(t_stop) | ||
local_t_eval = [t for t in T if current_t < t <= t_stop] | ||
|
||
print("Integrate bewtween ", current_t, " and ", t_stop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in last commit
control/__init__.py
Outdated
@@ -89,6 +89,7 @@ | |||
|
|||
# Allow access to phase_plane functions as ct.phaseplot.fcn or ct.pp.fcn | |||
from . import phaseplot as phaseplot | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason to add an extraneous blank line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, blank line is removed in the last commit
control/dde.py
Outdated
) | ||
|
||
|
||
def pchip_interp_u(T, U): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it is probably an internal function. If so, add underscore to start of function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
underscore added
control/dde.py
Outdated
return np.array([negative_wrapper(PchipInterpolator(T, ui)) for ui in U]) | ||
|
||
|
||
class DdeHistory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be better named DDEHistory
(since DDE is a standard abbreviation, like LTI)?
Also, if this is an internal class, begin the class name with an underscore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the name and added the underscore
control/delaylti.py
Outdated
Notes | ||
----- | ||
The way to create a DelayLTI object is by multiplying a transfert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: transfert → transfer
control/delaylti.py
Outdated
It's possible to MIMO delayed systems from arrays of SISO delayed systems, | ||
see function mimo_delay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to MIMO → It is possible to create
Also missing a period at the end.
control/delaylti.py
Outdated
|
||
# Poles and zeros functions for DelayLTI are not supported by julia ControlSystems.jl | ||
# might not be accurate for delayLTI, to be discussed | ||
# Here the poles and zeros computed are the ones of the system without taking into accoutn the delay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo + may be longer than 78 char (PEP8).
control/delaylti.py
Outdated
# Check dimensions of K | ||
if K.shape != (self.nu, self.ny): | ||
raise ValueError( | ||
f"Feedback gain K has incompatible shape. Expected ({self.nu}, {self.ny}), got {K.shape}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to avoid line with more than 78 characters and put closing parenthesis at end of string, for consistency with current code base.
control/delaylti.py
Outdated
def issiso(self): | ||
"""Check if the system is single-input, single-output.""" | ||
# Based on EXTERNAL dimensions | ||
return self.nu == 1 and self.ny == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled by the parent class (InputOutputSystem
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now moved to InputOutputSystem
This would be a great contribution, @MythasNauveili. Thanks for working on it! There are some issues with unit tests. In addition to the failed ruff and doctest checks, it looks like some of the standard tests might be hanging. Not sure if that is due to the new code or if something is messed up in the GitHub actions. I'll try to come back and take a look at that later. In the meantime, some style things to adjust in your next pass. |
Thanks for the first feedback @murrayrm. I tried to fix the most in the two last commits. For the tests hanging, it might be because of debug plots that I did not disable in |
@MythasNauveili There are still two unit tests failing. Click on the links above to see the issues. The ruff checks should be straightforward. For the doctest, it looks like something is printing out spurious information; not sure where that is happening. |
@murrayrm it might be ok now, I just removed a forgotten debug print in |
This PR introduces initial support for linear time-invariant (LTI) systems with time delays in
python-control
, inspired by the capabilities of ControlSystems.jl package, and theInternalDelay.py
script from Skogestad-Python.Key features
TransferFunction
object with adelay(tau)
object or anexp(-tau*s)
expression:PartitionedStateSpace
representation (implemented inpartitionedssp.py
). TheDelayLTI
class (indelaylti.py
) serves as the core object for these systems.DelayLTI
objects.Implementation Details:
solve_ivp
, was implemented indde.py
.control/julia/compute_tests.jl
) using ControlSystems.jl has been added to generate reference results, which are exported to a JSON file. These results are used in the test suite (control/tests/delay_lti_test.py
andcontrol/tests/dde_test.py
) to benchmark the accuracy of theDelayLTI
implementation and DDE solver.Current Scope and Limitations:
Discussion Points:
This PR represents the initial groundwork for this feature. I'm submitting this draft to share these first steps and would appreciate feedback on:
DelayLTI
,PartitionedStateSpace
).I'm aware there is still work to be done for this feature to be ready for a full release, but I believe this provides a solid foundation.