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

use type parameter for E matrix #12

Merged
merged 2 commits into from
Mar 10, 2022
Merged

Conversation

baggepinnen
Copy link
Contributor

If the field E is not associated with a type parameter, each access of E is type unstable, leading to a lot of inference problems and long compile times. This change brings a simple benchmark I have (using RobustAndOptimalControl which calls ghinfnorm in this package`) from

julia> using RobustAndOptimalControl, ControlSystems

julia> G = ssrand(2,2,2);

julia> @time hinfnorm2(G);
 18.141708 seconds (50.01 M allocations: 2.384 GiB, 8.45% gc time, 99.97% compilation time)

to

julia> @time hinfnorm2(G);
  6.451567 seconds (26.34 M allocations: 1.337 GiB, 7.95% gc time, 99.99% compilation time)

This is a step towards addressing the latest findings from

@baggepinnen
Copy link
Contributor Author

@andreasvarga CI will not automatically run on this PR since I'm a first-time contributor, there should be a button saying "approve" or something like that to press if you want to let CI run

@baggepinnen
Copy link
Contributor Author

While running the tests locally, I get failures for methods with signatures like

Base.hcat(systems::DST...) where DST <: DescriptorStateSpace

since it requires all systems to be of exactly the same type, and thus fails when trying to concatenate

DescriptorSystems.DescriptorStateSpace{Int64, LinearAlgebra.UniformScaling{Bool}}, ::DescriptorSystems.DescriptorStateSpace{Int64, Matrix{Int64}}

where one E::UniformScaling and the other E::Matrix. I'm not sure how you want to handle this, perhaps making the signatures less strict like this?

Base.hcat(systems::DescriptorStateSpace...)

@andreasvarga
Copy link
Owner

This seems to be an useful feature. Have you tried jointly with
Base.hcat(systems::DescriptorStateSpace...) ?

@baggepinnen
Copy link
Contributor Author

I haven't tried the suggested change, there were a lot of places that needed similar updates so I wanted to discuss the strategy before proceeding. I'll have a go at the suggested strategy tomorrow

accept systems of different types
@baggepinnen
Copy link
Contributor Author

With the latest commit, tests pass locally, and this simple compile-time benchmark goes from

julia> using DescriptorSystems;

julia> G = rss(2,2,4);

julia> @time ghinfnorm(G);
 20.182468 seconds (59.23 M allocations: 2.710 GiB, 5.65% gc time, 100.00% compilation time)

to

julia> @time ghinfnorm(G);
  6.971881 seconds (24.47 M allocations: 1.197 GiB, 10.94% gc time, 99.99% compilation time)

To verify that we didn't get any run-time regressions, here's a benchmark of the runtime as well
Before:

julia> using BenchmarkTools

julia> @btime ghinfnorm($G);
  1.535 μs (23 allocations: 2.11 KiB)

After:

julia> @btime ghinfnorm($G);
  1.419 μs (23 allocations: 2.20 KiB)

All tests were performed on julia Version 1.8.0-beta1

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #12 (1ea54fd) into main (9ab2700) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
- Coverage   97.02%   97.01%   -0.01%     
==========================================
  Files          17       17              
  Lines        3457     3455       -2     
==========================================
- Hits         3354     3352       -2     
  Misses        103      103              
Impacted Files Coverage Δ
src/connections.jl 94.78% <100.00%> (-0.05%) ⬇️
src/types/DescriptorStateSpace.jl 95.34% <100.00%> (ø)

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 9ab2700...1ea54fd. Read the comment docs.

@andreasvarga andreasvarga merged commit cbf9a0e into andreasvarga:main Mar 10, 2022
@baggepinnen baggepinnen deleted the typed-E branch March 10, 2022 10:51
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.

2 participants