-
Notifications
You must be signed in to change notification settings - Fork 58
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 rectangular matrix support for FullPivLu #164
base: master
Are you sure you want to change the base?
Conversation
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.
A few comments and questions. I'll defer more technical questions to those more knowledgeable :)
@@ -51,11 +51,17 @@ pub trait BaseMatrix<T>: Sized { | |||
/// Row stride in the matrix. | |||
fn row_stride(&self) -> usize; | |||
|
|||
/// Returns true if the matrix contais no elements | |||
/// Returns true if the matrix contains no elements |
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.
Thanks!
src/matrix/decomposition/lu.rs
Outdated
let r = self.rank(); | ||
let nonzero_u = self.lu.sub_slice([0, 0], r, r); | ||
|
||
let x = back_substitution(&nonzero_u, x.top(r)).unwrap(); |
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.
Do we mean to unwrap here? Wouldn't it be better to try!(back_substitution)
so that any errors are propagated to the result?
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.
Since nonzero_u
is full-rank and square by construction, I don't think back_substitution
will return an error as-is, but you're right, for future proofing and general code cleanliness, I think it should have a try!
. Thanks!
src/matrix/decomposition/lu.rs
Outdated
Ok(x) | ||
} else { | ||
Err(Error::new(ErrorKind::AlgebraFailure, | ||
"No solution exists")) |
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 error message should be a little clearer: "No solution exists for linear system"
- or something to that effect.
I also have to confess that I don't completely understand the solution validation method here. It seems sensible enough but I need some time to try and get some intuition.
src/matrix/decomposition/lu.rs
Outdated
} | ||
|
||
/// Multiplies an LU decomposed matrix by vector. | ||
impl<'a, 'b, T> Mul<&'b Vector<T>> for &'a FullPivLu<T> where T: Float { |
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 necessarily against adding this but I wonder what the use case is? (I'm sure one exists, I just haven't come across it personally).
Also - is there any reason we would want to limit this functionality to only the FullPivLU
(and not PartialPivLU
)?
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 also confused about the purpose of Mul
here! A clarification would be nice :-)
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.
The purpose of Mul
is to verify that the found candidate is an actual solution to the Ax=b
equation in solve
without having to clone and then unpack the current FullPivLu
struct. I didn't add it for PartialPivLu
because its solve
will always find a solution, since it's restricted to square full-rank matrices, so there's no verification required.
It's used on line 501: (self * &x - b)
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 see!
Nonetheless, having this functionality public seems a little untidy. I can't imagine a situation in which a user would need this and it makes the API a little inconsistent. Perhaps we should instead provide some private lup_mul
which is used internally for this part?
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.
Sure, totally! I will say that it seems fairly sane to me to allow a user to map a vector through a decomposed matrix without having to reconstruct it or clone the original prior to decomposing it, but if we think that the public Mul
clutters the API, I'd be happy to move it to a private internal function.
This is a very interesting PR! I'll do a thorough review later, but I want to have some high level discussion first. I must admit I hadn't even considered implementing
Let's consider a system Let now As far as I know, one typically uses a least-squares approach to these problems. In the case when If one is only interested in the exact solution and one has no interest in least-squares approximations, it might be possible to use LU decomposition, although I've never seen this used for this purpose (but keep in mind I'm not at all an expert). From a brief glance, I'm intrigued by your technique. Without having carefully analyzed it, it raises some questions:
I'm open to providing a
From my understanding by looking at the code, I think the proposed |
I totally agree with @Andlon that if The approach taken in the PR is to set all the free variables to zero, and solve the equation using just the upper-left I think that this implementation meets the criteria set out by @Andlon. Am I missing something? |
@Prismatica this makes sense to me though my opinion weighs less heavily than @Andlon and your own on this. |
@Prismatica: I took a closer look now, and yes, I believe you are right! You only described the underdetermined situation, however (where U is rectangular and L is square). We must also consider the case where the system is overdetermined, in which case L is rectangular and U is square. For clarity, I'll try to outline a generalized procedure below, based on your approach. Please let me know if I've made any mistakes! I think I might also have found a somewhat better way to verify the solution (?). Problem statementWe wish to find any solution to the problem The introduction of the size Existence of solutionsWe now write
where Because
Hence, if there exists a solution to the problem Solutions to
|
Thanks for the clear write-up, @Andlon! I might be misunderstanding a few steps in your derivation, so I have a question: My impression is that, to a certain extent, one can choose the size of the If this can be done, I think the |
Well, let's say that we want to have
for any matrix Note that as our "LU matrix" that we produce during The approach involving a full square matrix Does this make sense? Please ask if I haven't been clear! |
That does make sense, thanks! Unfortunately, real life has caught up with me, and I won't be able to give this the time I'd like in the next few weeks. I'll try to get a new version of this that implements your changes up as soon as I can, but if it takes longer than it has in the past, I promise I haven't forgotten about you all :) |
@Prismatica: no worries! Please take as much time as you need. This PR is also very independent of other ongoing work in the library, so there should be no issues as a result :-) |
Alright, sorry, I have another potentially stupid question. You say:
How does one compute |
Previously, the L matrix would always be a mxm matrix, and the U matrix would be a mxn matrix. This is because the unpack method used the self.lu matrix exclusively to store the U matrix. In order to avoid wasting large amounts of memory, the internal storage is repurposed to store either the L or U matrix, whichever can fit into the mxn size. The other matrix is allocated as a square matrix. Also removed the Mul trait for FullPivLu, and moved that trait to a private internal function.
This most recent commit doesn't address all of @Andlon's concerns. It just changes how Also, I removed the I'm still trying to understand @Andlon's algorithm, though it's going slowly (my own fault, of course!) Thank you all for your patience! |
@Prismatica: there are no stupid questions here, so please don't apologize! And if there were, I'd surely be the champion of foolish questions under normal circumstances :-) Sorry, I was not so explicit with regards to the meaning of some of the quantities I introduced in the above description. We define There is/was actually a typo in the original "algorithm" that I presented: we need to compute
I understand your confusion. This is not it's definition, but the requirement that I hope this helps! Please don't hesitate to ask more questions. I think we're all learning from this :-) |
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.
Just a very minor point. I'm also still trying to understand the final implementation but this work is really exciting!
Keep it up and feel free to ask any questions. I'm also learning a lot from this thread.
src/matrix/decomposition/lu.rs
Outdated
let l = unit_lower_triangular_part(&self.lu); | ||
let mut u = self.lu; | ||
nullify_lower_triangular_part(&mut u); | ||
use internal_utils::nullify_upper_triangular_part; |
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.
Looks like this import has been accidentally doubled.
We should also avoid locally importing. If this is needed here we should import it at the top of the file. This just makes it easy to manage the imports and change module structure in future.
I've updated the code to what I think is the algorithm listed above, but I think I must be missing something. For example, let's take the case of
Then,
Then, since I think this means that Since the rank is 1, Is there an additional verification step required, such as the explicit map through like in the original implementation, or have I missed something again? |
It may be that there are mistakes in the algorithm I proposed, but I think it works for this particular example at least! Let's go through it step by step.
Since To see that the system actually has no solution, let's write it out with variables. The system
If we make I went through this a bit quickly, so it's very possible I've made mistakes, but I hope it should be correct. I hope this helps. Please don't hesitate to ask more questions! |
This fixes a dumb bug in the previous commit. Instead of using diag_size, the verification step should use the rank. This applies to the full-pivot rectangular solve routine.
OK, I finally figured it out. My apologies for all the back-and-forth and delays, and thanks for your help and patience! :) Please let me know if there's something else I should change. |
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.
Great work on this PR, @Prismatica! This PR introduces a novel non-standard feature to the library, so I think it justifies a little discussion :-)
Some thoughts and points of discussion:
- Should we perhaps consider not returning a
Result
fromdecompose
, because it should never fail? Note that this might be a breaking change (though at the momentFullPivLu
is only on master and not in any release, so should be safe) so we can postpone it to a later PR too. - I think the documentation for
solve
should in some limited sense try to explain what it actually does in the face of non-invertible matrices. - I also think that since the algorithm employed in
solve
is not exactly obvious, we might want to include a short summary of the ideas and steps that we've discussed in this PR as an inline comment in the implementation ofsolve
- This one is tricky: Currently only an absolute-valued epsilon tolerance is used to determine if the number is (approximately) zero. However, this may break down depending on the scaling of the matrix or the right-hand side. I think we need to look a little deeper into a suitable criterion here. I will think about this.
- Performance: There are a few places where a
Vector
is somewhat unnecessarily allocated. I've made note of this in my comments below. To be clear, I think as a first stab at the implementation you've done great! However there is some room for improvement in avoiding redundant allocation. We can also push this back to a later PR. - I think it's important that the "happy path" is optimized. In other words, I think it's important that when solving for an invertible matrix, the operation is as fast as possible (within reason).
There are at least two ways to tackle my last point:
- Branch on
is_invertible()
and solve the invertible problem directly. - Optimize the procedures involved in such a way that when the matrix is invertible, a minimal amount of work is performed, such that no branching is necessary.
The second idea is clearly favorable, but it may be more tricky to implement in practice. However, I think combined with the work necessary to avoid allocating redundant Vector
instances, this may turn out to fall out as a biproduct.
I've made a lot of comments here, but please note that this doesn't mean that everything needs to change. My opinion is certainly not gospel :-) As the implementer and the person who has spent the most time thinking about this problem, I'd be very interested to hear your thoughts!
// at least one of the matrices, we need to figure out which | ||
// matrix to store in self.lu, and which to allocate anew. The | ||
// nice thing here is that the new allocation will always be | ||
// PxP, which is the smaller of the two matrices. |
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.
Love this comment! Explains the idea very well.
let x = | ||
lu_forward_substitution( | ||
&self.lu.sub_slice([0, 0], diag_size, diag_size), | ||
b.top(diag_size)); |
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.
From a performance point of view, it would be neat not to require us to copy the "top" of b
into a new vector. Can we perhaps consider rewriting lu_forward_substitution
to take a standard slice instead of a Vector
?
@@ -469,8 +496,59 @@ impl<T> FullPivLu<T> where T: Any + Float { | |||
assert!(b.size() == self.lu.rows(), |
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 maybe the docs need to be updated so that it reflects what this function does in the event that the system is not invertible.
let val = | ||
utils::dot( | ||
&self.lu.data()[row_begin .. row_end], | ||
&x.data()[0 .. diag_size]); |
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.
This part can perhaps be more easily rewritten using row_iter().enumerate().skip(rank)
instead! You can further use row.as_raw_slice()
to directly obtain a slice of the row.
// Consider the top-left square invertible portion of U. | ||
let nonzero_u = self.lu.sub_slice([0, 0], rank, rank); | ||
|
||
let x = back_substitution(&nonzero_u, x.top(rank)).unwrap(); |
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.
We might also want to consider rewriting back_substitution
to take a slice instead in order to avoid the copy of the vector data here, but this will affect more than just LU though.
// the last permutation matrix. | ||
let x = Vector::from_fn( | ||
self.lu.cols(), | ||
|i| if i < rank { unsafe{ *x.get_unchecked(i) }} else { T::zero() }); |
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.
Can we perhaps avoid the unsafe block here?
let diag_last = self.lu.rows() - 1; | ||
let last = self.lu.get([diag_last, diag_last]).unwrap(); | ||
|
||
last.abs() > self.epsilon() |
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.
self.lu.rows() - 1
will underflow if self.lu.rows() == 0
, which I guess can happen in this case. Yeah, I know this is annoying :-/
size: count, | ||
data: self.data[0..count].to_vec() | ||
} | ||
} |
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 agree that this might be a useful addition, but I think in light of #168, we might want to avoid adding any new methods to Vector
at the moment.
This is an attempt to resolve issue #161.
I'm pretty uncertain about the decisions I made in
FullPivLu::solve
. The main problem is that, unlike inPartialPivLu
, which requires square invertible matrices,FullPivLu::solve
can now be given a problem that has no solution or has infinitely many solutions. Thoughts on how I went about dealing with this? The other option I saw was to require the matrix be square and invertible forFullPivLu::solve
.