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

Crating discreate delay system is not allowed as delay is continues #773

Closed
jamestjsp opened this issue Nov 17, 2022 · 2 comments · Fixed by #774
Closed

Crating discreate delay system is not allowed as delay is continues #773

jamestjsp opened this issue Nov 17, 2022 · 2 comments · Fixed by #774

Comments

@jamestjsp
Copy link

when delay block is multiplied with a discreet Transferfunction the following error will be thrown.
ERROR: MethodError: no method matching Continuous(::Discrete{Int64})

Workaround:
Create a discreet delay using c2d(delay(L), Td) then multiply with discreet Transferfunction.

It is good to have this done automatically.

@albheim
Copy link
Member

albheim commented Nov 17, 2022

Since delay creates a DelayLtiSystem this suggestion would mean a discrete system multiplied by any general DelayLtiSystem would result in an automatic discretization.

I feel like discretizing general systems automatically is maybe not a good idea and should be something that is explicitly done by the programmer to avoid mistakes, thus my preference would be that it errors, but maybe the error message could be improved a bit. Though I can't say my stake in this is big, so don't listen too much to me.

@baggepinnen
Copy link
Member

It is good to have this done automatically.

I do not agree, continuous time systems and discrete-time systems are fundamentally different, and implicit conversion is a foot gun that we'd rather avoid. I thus agree with Albin that we should keep discretization an explicit action by the user.

#774 instead adds a discrete-time constructor for delay to make this smoother! Thanks for the issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants