Implementation reverse Cuthill-McKee

[lksriemer]

See also #185 .
This changes the Cuthill-McKee algorithm implemented here to the reverse Cuthill-McKee algorithm. Starting vertices are choosen by a George-Liu pseudoperipheral vertex finder.
I have added two more test cases and verified the correctness of the exisiting one w.r.t. the new algorithm.

I did try to write in the style of the library, however I might not always have got it right when to use .index(), when to call .unwrap() vs .expect(), and so on. Feel free to change these details, as well as remove unneccesary comments or line breaks. I just can't get enough of those.

As benchmark possibilities are scarce, I did not optimize anything to aggressively. I really believe this crate should come with some sparse matrix collection as well as random matrices to benchmark against, automatically. That would make tweaking the details worthwile. That's an issue for another time though.

Concerning the minimum degree ordering: As it is significantly more complex, It will take some more time to implement it, as in a few days to weeks. I will do it eventually, but I don't want to block somebody else on it, if there is interest.

[vbarrielle]

That looks nice, thanks.

First remarks:

• I think it would be better to expose both cuthil_mckee and reverse_cuthill_mckee
• could you make sure the comments are wrapped at the 80 characters limit? Running cargo fmt should do it automatically. (I'm wondering why no CI launched on your PR?)

I'll take more time to review the rest a bit later.

[vbarrielle]

My bad, looks like CI did pass https://travis-ci.org/github/vbarrielle/sprs/builds/668095347, I don't know why github did not display it...

[lksriemer]

I am wondering why CI was not displayed as well, but I can't help you there, I am not that good with Github.

• Exposing cuthill_mckee too ... I don't know. It is easily simulated by just reversing the reverse order. Sure, that is definitely not optimal from a performance standpoint, but it shouldn't matter too much, as, firstly, the normal ordering should only be used rarely, and secondly, those O(n) algorithms probably aren't bottlenecks anyway. Still, if you really want it too, that's totally fine by me.

• I contemplated if maybe the algorithm should be generic over a StartingStrategy. Then the consumer of the algorithm could decide on how to pick a starting node. Some options that immediately come to mind: PseudoPeripher, MinimumDegree, Next, Constant. Being able to optimally choose depending on the specific situation seems like a strong point to me. What do you think?

• I did run cargo fmt, no idea why these style guidelines were not enforced. Seems like a problem with the project configuration. I am using VS Code on Windows, even manually running rustfmt with passed --config max_width=80 option didn't format the comments correctly. Any idea what to do?

Take your time to review, though the tests seem to pass I am not sure if I got all the library-specifics absolutely correct.

[vbarrielle]

Don't worry about the CI, maybe it's simply an issue with github.

The main reason why I wanted to expose cuthill_mckee as well was to have an API exposing all the choices. It's quite a simple operation indeed, but i feel it's nice to present the user with all the possibilities.

In that spirit, I think it's a idea to expose the StartingStrategy, especially if the StartingStrategy is made to implement Default.

Don't worry about the comments line width, I'll fix them myself later on, and I'll try and find the proper rustmf configuration.

[vbarrielle]

I'm done reviewing. I've really appreciated reading your code, the style is very clear and well commented. I mentionned a few places where I'd like to see extra clarifications.

I haven't read the paper on the algorithm you've implemented for the pseudoperipheral vertex, so I'll trust you on that one for the moment, but everything looks reasonable in the implementation.

Thanks a lot for doing this!

[lksriemer]

I will push the requested changes now.

After giving it some more thought, I believe the best compromise between ease of use and customizability is: exposing one function generic over the orderings direction and the starting strategy, cuthill_mckee_custom(mat, strategy, direction), along with one function reverse_cuthill_mckee(mat), which essentialy calls the custom function with the defaults as specified in this PR. I will take a day or so to push this customizable algorithm. Don't merge yet ;-)

[vbarrielle]

I agree exposing two functions is the best way to enable choosing the init strategy while encouraging pseudoperipheral as a default.

[lksriemer]

Here is the refactored, extended version.
I decided to favour traits for the options. For the strategies that is the obvious choice, for the orderings direction not so much. However I do not see an other way to create this abstraction without considerable runtime costs.

Concerning the namespacing, that definitely needs to be overhauled. I introduced the modules start and order, but they need to be publicly re-exported in some sensible way or something. It's up to you to decide if and how to do that. Also, maybe a different file structure is needed.

Please comment what you would like to be clarfied etc., and if you believe there need to be more test cases covering all the different options, although most are quite trivial.

[vbarrielle]

Even though the trait abstraction makes the code a little harder to follow, I agree with the motivation for monomorphization. Your comments are to the point and make the implementation quite readable.

In addition to the points I mentionned in my review comments, I think it could be nice to show an example of custom re-ordering in the fill_in_reduction.rs example. That way we can see if the module structure feels ergonomic.

After this I think it will be ready to be merged.

[lksriemer]

I have implemented all changes as requested. If there is nothing left to discuss, this can now be merged.

[vbarrielle]

That's perfect, I'm merging.

[vbarrielle]

ANd thanks a lot!

[lksriemer]

And thanks a lot!

It's the magic of open source!