-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add pencil types #187
Add pencil types #187
Conversation
058882f
to
c5951da
Compare
Co-authored-by: Christian Schilling <[email protected]>
src/pencils.jl
Outdated
|
||
IndexStyle(::Type{<:IntervalMatrixPencil}) = IndexLinear() | ||
size(M::IntervalMatrixPencil) = size(M.A0) | ||
getindex(M::IntervalMatrixPencil, i::Int) = getindex(M.A0, i) + M.λ * getindex(M.A1, i) |
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.
I'm not sure this is useful. Thinking about e.g. the skalna06 algorithm, it would make more sense to access the coefficient(s) of the variable(s) to be able to manipulate those.
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.
I think this implementation results in what you would expect. For instance, if you print the IntervalMatrixPencil
, I think it will use getindex
. (Side note: is it maybe more useful to overwrite show
to print the individual matrices?)
To achieve what you suggest, you would do getindex
on M.λ
. Maybe there should be a getter function getparam(M)
(and a general version with the index getparam(M, i)
).
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.
I would indeed expect the pencil to be printed with
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.
I'm not sure this is useful.
this is the canonical way of accessing the matrix indices
the coefficient(s) of the variable(s)
which variables?
I would indeed expect the pencil to be printed with
$\lambda$ (or other variable) as undeterminate
in this implementation, \lambda is not an indeterminate. it is a known interval.
I mean if you don't have an undetermined variable it's not a pencil
hmmm to me this type is just a structured way of holding A + lambda B without distributing the sums
shall go back to the whiteboard? i've opened #191 (comment)
src/pencils.jl
Outdated
|
||
IndexStyle(::Type{<:AffineIntervalMatrix}) = IndexLinear() | ||
size(M::AffineIntervalMatrix) = size(M.A0) | ||
getindex(M::AffineIntervalMatrix, i::Int) = getindex(M.A0, i) + sum(M.λ[k] * getindex(M.A[k], i) for k in eachindex(M.λ)) |
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.
same of before
src/pencils.jl
Outdated
struct IntervalMatrixPencil{T, IT, MT0<:AbstractMatrix{T}, MT1<:AbstractMatrix{T}} <: AbstractIntervalMatrix{IT} | ||
A0::MT0 | ||
A1::MT1 | ||
λ::IT |
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.
I am not convinced caching the interval is that useful. I have nothing against having a default domain for the parameters cached, but I think the interface should offer an easy way to evaluate the parametric matrix at different points and/or over different intervals.
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.
Indeed I think it would make more sense to keep the "domain" of the parameters separated from the parametric structure, it would also make easier to test the parametric solver over different interval ranges
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.
For example, it would be good to make those objects callable, so that I could do P(0.4)
or P(1..2)
so that I could evaluate the matrices at arbitrary points. At that point, I think it would be acceptable to cache a "default" interval and have P()
evaluate the pencil (affine matrix) there, although I think this could feel a little confusing.
src/pencils.jl
Outdated
""" | ||
struct IntervalMatrixPencil{T, IT, MT0<:AbstractMatrix{T}, MT1<:AbstractMatrix{T}} <: AbstractIntervalMatrix{IT} | ||
A0::MT0 | ||
A1::MT1 |
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.
I am not sure whether it makes more sense to have a vector of matrices (as here) or a matrix of vectors. I think the latter would make it easier to evaluate single expressions at given points?
@lucaferranti: I think you suggest to have a type for a truly parametric pencil (resp. affine matrix) and then an instantiation with a parameter domain. I cannot yet see when you actually want to use the parametric pencil, but it makes sense to me. So I think the suggestion would be to introduce four types: As long as we do not rely on field access but use getter functions, we could also add the parametric types later. |
you need to access the parametric structure to solve parametric interval linear systems. (Ok to be fair, I haven't seen PILS solver optimised for pencils, so if your comment strictly meant, I agree that you want the parametric version for the affine matrix, but I don't see why you need that also for the pencil, than off the top of my head I cannot throw an example to contradict that, except that it's not a pencil if you don't have an indeterminate variable. I think the current structure would also make montecarlo simulations and evaluations at single points a little harder. |
Introducing matrix functions a la matrix pencils is deferred to #191 |
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.
LGTM! I have one minor comment, since the docstrings mention that the coefficient matrices can be complex, should there be 1-2 tests for it?
No description provided.