Skip to content

Conversation

@baggepinnen
Copy link
Member

This PR "soft deprecates" the matrix-equation solver interfaces that distinguish between continuous/discrete time by means of a prefix c/d in favor of function that dispatch on the TimeEvol type.

lqr(...)   -> lqr(Continuous, ...) or lqr(dsys, ...)
dlqr(...)  -> lqr(Discrete, ...)   or lqr(csys, ...)
dlyap(...) -> lyap(Discrete, ...)

and so on. This avoids the previously common

if isdiscrete
    ared
else
    arec
end

Instead, we may now use

are(te, ...)

where te is the sys.timeevol field (type or instance, both work).

I say "soft deprecate" since the old methods are still there, their use is just discouraged in the docstring.

Supersedes #563

@JuliaControlBot
Copy link

Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/actions/runs/1773457593?check_suite_focus=true for more details.

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #629 (b792828) into master (6b8e2d0) will increase coverage by 0.06%.
The diff coverage is 82.60%.

❗ Current head b792828 differs from pull request most recent head dd6a35c. Consider uploading reports for the commit dd6a35c to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
+ Coverage   86.66%   86.73%   +0.06%     
==========================================
  Files          31       31              
  Lines        3233     3248      +15     
==========================================
+ Hits         2802     2817      +15     
  Misses        431      431              
Impacted Files Coverage Δ
src/ControlSystems.jl 100.00% <ø> (ø)
src/types/TimeEvolution.jl 83.87% <ø> (ø)
src/matrix_comps.jl 92.63% <72.72%> (-0.37%) ⬇️
src/synthesis.jl 88.09% <90.00%> (+2.09%) ⬆️
src/types/Lti.jl 50.00% <100.00%> (-5.56%) ⬇️
src/timeresp.jl 89.68% <0.00%> (ø)
src/types/result_types.jl 89.47% <0.00%> (ø)
src/analysis.jl 87.21% <0.00%> (+0.04%) ⬆️
src/freqresp.jl 96.24% <0.00%> (+0.17%) ⬆️
src/plotting.jl 85.59% <0.00%> (+0.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b8e2d0...dd6a35c. Read the comment docs.

Q = I
R = I
L = dlqr(A,B,Q,R) # lqr(sys,Q,R) can also be used
L = lqr(Discrete,A,B,Q,R) # lqr(sys,Q,R) can also be used
Copy link
Member

Choose a reason for hiding this comment

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

I haven't really followed the discussion about these changes, but to me it feels slightly strange with Discrete as first argument. But I guess you always have to supply an argument, we don't want to make one default?

The other one where you send in a system looks nice, so maybe it makes sense that the system information comes in with the leftmost argument in some type of consistency with that.

Nothing I have strong feelings about, just reacted to it and unsure if there are good alternatives (I don't have any that I think are really better)

Copy link
Member Author

Choose a reason for hiding this comment

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

Having one as default is part of what we're trying to get away from, since lqr(sys) and lqr(A, B) then not mean the same if the system is discrete. The idea here was to use the name lqr rather than lqr/lqrd and at the same time allow for dispatch on sys.timeevol rather than having manual if isdiscrete(sys) everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense.

This function exists for legacy reasons, users are encouraged to use the interface `are(Continuous, A, B, Q, R)` instead.
"""
function care(A, B, Q, R)
function are(::ContinuousType, A::AbstractMatrix, B, Q, R)
Copy link
Member

Choose a reason for hiding this comment

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

Why type on A but not the others?

Copy link
Member

Choose a reason for hiding this comment

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

I assume there is a reason since I see it is done on a few more spots, just curios and want to understand :)

Copy link
Member Author

Choose a reason for hiding this comment

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

If I do not type the A, there is a method ambiguity when A isa Number between this method and the method that takes are(t::TimeEvolType, A::Number, ...), since TimeEvolType is less strict than ContinuousType.

Copy link
Member

@albheim albheim left a comment

Choose a reason for hiding this comment

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

LGTM

@baggepinnen baggepinnen merged commit f368360 into master Feb 16, 2022
@baggepinnen baggepinnen deleted the timeevol_me branch February 16, 2022 07:03
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 this pull request may close these issues.

4 participants