Issue 06/fix linting

[MothNik]

This pull request implements the lint suggestions given when running cargo clippy:

• ➿ replaces loops by iterators (vector fills and some more complex loops)
• ✍️ removes fn main {...} from docstrings (all in one commit; can be undone if required)
• 🍰 changed static typing from vectors Vec<something> to slices [something]
• 🔄️ removed unnecessary referencing
• 🎁 removed unnecessary returns (it is encouraged to not use them even though I think this is debatable)
• 🔢 removed unnecessary !vec from tests

Besides, it makes some tiny changes to make it more idiomatic here and there (mostly using match or map_or for some if x.is_some()) and also converts some highly predictable while- to explicit for-loops (even though the compiler probably handled this gracefully all on its own).
For the householder reflections 🪞 it flips the order of the if sigma == 0 and the if sigma != 0 because the latter one is the more likely one to go. It should thus be the first in the if else to make sure the CPU branch predictions are made properly by guessing it as the more likely branch first (⚠️ although a float-comparison with ==0 is mostly mathematically inspired, but due to floating point imprecisions this should probably be handled with some tolerances in the code instead; left a FIXME).

On top of that, it fixes a 🔤📖 typo here and there.

✅ The tests are all passing
⏱️ From a runtime perspective, the solver tests went from ~150 seconds execution time to ~120 seconds for a release build, but those timings were taken on a laptop, so take them with a grain of salt 🧂

[RLado]

Minor fix: This code block closes twice

[MothNik]

Thanks for pointing this out!
Fixed by this commit

Tests still pass ✅

[RLado]

Thank you for such a well-organized pull request! The changes clearly improve the code, and indeed, the tests run slightly faster on my machine as well ( ~2.302s -> ~2.260s ).

I am not a big fan of the auto-formatting on the test files because it adds a lot of new lines and makes the code slightly more inconvenient to navigate (at least for me). Nevertheless, I will admit that now the formatting is more consistent, so... Not really complaining.

This pull request introduces breaking changes with respect to version 0.2.3; however, functionality has remained the same for the past year. I think it might be time to release v1.0.0 once these changes are merged.

Once again, thank you for pointing out the errors in the code and taking the time to fix them. I appreciate the effort.

EDIT: I was reviewing the code for a 1.0.0 release and this pull request is perfectly compatible with 0.2.3.

[MothNik]

Thanks for the positive feedback! 😊

I hope the reformatting of the tests does not make it too inconvenient 😖

Happy to hear that! There is not that many crates which went beyond the 0. major version 👏

Just out of curiositiy. Do you refer to the doctests only for the 2 seconds?
Because the whole test suite is like ~150 seconds for me.

[RLado]

The format will not be a problem! I am merging the pull request 🥳

About the tests, the full test suite runs in 2 seconds on my machine (Ryzen 7800X3D) when I run cargo test --release. Did you run the tests with the release flag?

[MothNik]

Nice 🥳

I didn't know that there is also a --release-flag for the tests, too. Good to know, thanks! 😅
Now, it runs in 10 seconds 🏃 but I just have an old AMD Ryzen 7 3700U and not such a fancy CPU 🤩

[MothNik]

Maybe before going to major version 1:
Do you think it makes sense to

• enrich Nmrc by the the method it was computed from (order; would not break anything) as well as computable quantities like the log-determinant and/or the main diagonal elements of the inverse (can be used for parameter tuning in, e.g., smoothing algorithms that rely on sparse factorization). The first log-determinant would be quite straightforward, the main diagonal would be more invested and probably used less. Maybe you also have other ideas.
• add functions like sol_from_qr, sol_from_lu, sol_from_chol that take an already available Nmrc which would allow for solving multiple right hand sides. qrsol, lusol, and cholsol could then be meta-functions that just call, e.g., sqr, qr and sol_from_qr respectively. Like this, they could also take multiple right hand sides (would also not break anything but just rearrange the internal logics).

I would really appreciate to hear your opinion on this. The first point is more an optional nice-to-have, though 😅

For the second point, the linear algebra library LAPACK (Fortran) offers the same functionality. For, e.g., banded Cholesky-based solvers, it offers:

• pbtrf to compute the Cholesky factorization from a matrix A
• pbtrs to use such a pre-computed Cholesky factorization from A to solve one or multiple right hand sides
• pbsv which first factorizes A and then solves one or multiple right hand sides using this factorization

So, something like this would be a standard way this could be approached 🤔

[RLado]

The initial scope of this project was to implement this book in Rust because I was building a thermal simulator for a specific academic application and I could not find a Rust solver that satisfied my needs (also, it was a good project for learning Rust). After I finished implementing the current set of features described in the book, I was not planning on building new features. In part because I have moved on from that project to other stuff.

However, your proposal is in line with what this crate is, and so I am not against widening the scope of rsparse. Unfortunately, right now I do not have the time to implement those features and ensure they work correctly. If you are willing to build them, I would be glad to include them in the project. That said, this is a small project, and I am not sure I would be offering much value as a maintainer.