Cleaned up formatting. #57
Conversation
| // For a matrix M, each singular value σ and left and right singular vectors u and v respectively | ||
| // satisfy M v = σ u, so we take the difference | ||
| // For a matrix M, each singular value σ and left and right singular vectors u and v respectively | ||
| // satisfy M v = σ u, so we take the difference |
There was a problem hiding this comment.
Seems like a case unhandled by rustfmt? In-line comments should be indented when chaining methods, imo! Not so important, but worth taking note and potentially make an issue in the rustfmt repo? What do you think?
|
I think this is definitely a good idea. @AtheMathmo: could we consider making |
| if diag.abs() < T::min_positive_value() + | ||
| T::min_positive_value() | ||
| { | ||
| if diag.abs() < T::min_positive_value() + T::min_positive_value() { |
There was a problem hiding this comment.
This has nothing to do with rustfmt, but I just noticed that this condition needs to be improved, although I'm not sure exactly what the right check should be.
There was a problem hiding this comment.
Looks like this is for back substitution solving - to check if the matrix is singular. It probably exists in forward substitution too... We can use the handy new MachineEpsilon trait it seems :).
There was a problem hiding this comment.
So 2 * eps should be a sufficiently accurate bound? I really have to get around to immersing myself more into the corner cases of floating point arithmetic soon...
There was a problem hiding this comment.
I am assuming it would be yes - though really I have very little idea myself! There may be a more technically complete way to do this.
There was a problem hiding this comment.
I'm not sure, but there may be more accurate bounds when comparing with identically zero. Just want to make sure that we're not discarding matrices that are actually nonsingular as singular because our bound is too loose!
Should do some research on this...
EDIT: bound too low -> loose
There was a problem hiding this comment.
I opened #58 - maybe my terminology is backwards in the title though...
|
Thank you for doing this! I'm certainly a bit lax with rustfmt... In terms of enforcing as part of the contribution process I think it might be a good idea but I'm not entirely sure how. Another issue is that contributors frequently have different settings and so our rustfmts conflict with eachother - we can solve this by having a config file in the project root though. There are a couple issues I've had with rustfmt that we should probably bring to the attention of the developers...
I'll merge this after your other PR. |
|
If we have such a config file, would it be very hard to get the CI to run |
|
Ah, see here. Seems very plausible, actually! |
|
We should definitely look into this - but I'm not sure if it is viable right now. I checked and for me the parser still breaks on long lines. Some of these lines we can't reduce as they are long urls to reference material. Maybe we can get around this with a custom config.. |
Yeah, I guess this is not the most pressing issue anyway. Perhaps we could establish a long-term tracking issue for something we'd like to converge towards? |
|
Yes we probably should. I'll add after we merge this (in case any more information comes from this). @nwtnian I am happy to merge this PR as is - however on my local machine I'm seeing a lot more changes when I run rustfmt. Even if I add the option to wrap long comments my rustfmt still fails on some longer code lines and I see changes in many more files than those affected here. Could you give a little more detail on your set up? Which version of rustfmt are you using and do you have a custom config? |
|
@nwtnian pinging you on this again. There are some conflicts now from where this has been sitting. Once we can converge on the rustfmt settings used I'll be happy to merge this. The issue currently is that my rustfmt still modifies these files that you have changed. |
|
Am closing this as it seems abandoned to me - will happily reopen if requested. Am also creating a new issue to standardize our formatting. |
…rnings Clean up compiler warnings
Cleaned up formatting by running
$ rustfmton rulinalg.