Open KCErb opened 5 years ago
require "linalg"
m = LA::GMat[[3, -2],[4, -1]] vals, vecs = m.eigs
res = vecs.inv m vecs res.detect(MatrixFlags::UpperTriangular | MatrixFlags::LowerTriangular)
This checks only flags you passed and returns true if all flags are set. Well, it's still more verbose than your suggestion because of Enum syntax in Crystal. It's not named `detect?` because yes, it does not only returns result but also caches it in a matrix.
`detect` checks all flags and this can be slow (`PositiveDefinite` involves decomposition and `Orthogonal` involves multiplication). (I've no idea whether it is important though, maybe it doesn't matter).
That said, I agree that your idea is better - `detect` could be useful in chained expressions, and if someone cares about speed he can pass required flags.
2. This looks very nice!
We still can't forward all methods (using `forward_missing_to`) because there are some helpers, but it's not a problem to specify methods manually.
I hadn't thought of the performance cost for some of those flag checks, that's a very good point.
In my specific use case all I really care about is the diagonal?
method, perhaps the better solution is to give Mat
a diagonal?
method which checks for the flag, and in its absence runs a detect.
That sort of pattern is probably extensible to other flags.
As for detect?
being mutating, the side effect is completely harmless right? If a matrix possesses the property in question, there can't be any harm in that being noted in the flags. So it seems acceptable to me. Though I imagine for my use case, where I was only calling detect because I wanted the side effects, the more idiomatic would be detect!
.
Given all of that, I think I'd personally like
mat.detect!(flags = LA::MatrixFlags::All)
for modifying the matrix, this returns the matrix.
mat.diagonal?
for specifically checking and setting one flag
and
mat.detect?(flag : LA::MatrixFlags)
for checking for a flag, setting it if available, and returning a boolean.
What do you think? Too much?
Checking and detecting won't work in current state, because only "positive" result is cached. That is: if detect
sees that matrix is diagonal, it sets the "diagonal" flag (and further operations will take it into account). But there is no "non-diagonal" flag, so for a non-diagonal matrix it will check diagonality with every call of diagonal?
. Perhaps the system should be modified to include negative results though - mat.diagonal?
without it is just misleading (flags.diagonal?
can return false on diagonal matrix if we forgot to call detect
).
As for detect!
vs detect
- there is also assume!(flag)
- unsafe method, for setting a flag without check (when you know that it is present and don't want to waste time to check). So i decided to name detect
without !
for a contrast.
So to summarize: currently present detect
should be renamed to detect?
, detect
(or detect!
) should return a matrix for chaining, and mat.diagonal?
is very nice, but requires changing flags system, so it will take some time.
I'd be fine with not having a "negative flag". It makes sense to me that each call of diagonal?
would have to actually look through the matrix for entries if the flag is not already there. In general, I'd expect any flag operation to have to actually introspect on the matrix because you only have two states "I'm sure the matrix has this property" and "I'm not sure". I like the simplicity.
The more complex situation is a 3 state matrix for each flag "definitely yes, definitely no, unsure". But then I don't feel like enum's are the right pattern, or they could be, but you'd have a new set of flags for each property.
The only way we could have a two state "definitely yes, definitely no" would be if we were actually calculating all flags on every operation which is obviously not a great idea.
So I agree with most of what you said, but don't see the need to rework flags ... but that's just my two cents :)
I would have liked my sample in #7 to be a tiny bit more condensed
It'd be nice if
detect
returned the input matrix, since it adds flags to that matrix. If a boolean would be a nicer return perhaps we could usedetect?
instead. But I suspect the only use of boolean is in checking the flags directly, detect seems to be for adjusting flags after a set of operations. And of course for that we can just usematrix.flags.property?
It'd be nice if the Flags enum methods were forwarded to the matrix, so that instead of
matrix.flags.diagonal?
I could just callmatrix.diagonal?
.