Optimize DisjointSet

[gmou3]

The methods find and union of the class DisjointSet_of_hashables pass through an unnecessary layer of input checks that slow down the code. I avoided these checks, cythonized more of the code, and edited the docstrings.

I can attest to the fact that these changes improve speed, and in a follow-up PR (#37839) I mention a test that confirms this.

[github-actions]

Documentation preview for this PR (built with commit e24a492; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[mantepse]

Wouldn't it make sense to simply remove the input checks?

(I think not, but I cannot really substantiate my intuition. It may make sense to leave them in, because cython is essentially not debugable.)

[gmou3]

Yes, I would prefer to remove them altogether. However, union is a reserved word in C, so I was tempted to create these alternatives. Any suggestions welcome.

[tscrim]

Here's some things to try and make it a bit faster.

[gmou3]

@mantepse I only kept the version with no input checks and renamed union to join. Note that the Disjoint Set data structure is also called a union-find data structure or merge-find data structure (is merge a better name? - see https://en.wikipedia.org/wiki/Disjoint-set_data_structure).

@tscrim Thanks for the suggestions; I want to make it as fast as possible. I did implement the recommended changes and I think it is a tiny bit faster. (It is not easy to come up with good tests.)

[tscrim]

Thank you. Although I am -1 on renaming union to join. I believe union is a fairly standard name for this and a name conflict with a reserved C keyword is a poor argument IMO (as we are in Cython/Python and it is a method, which can have the same name as reserved keywords).

[mantepse]

Yes, please keep union-find.

[gmou3]

Ok, I managed a workaround and now union works.

[videlec]

Calling OP_find(OrbitPartition * p, int n) without being sure that n is in the range of p might result in a SEGFAULT. While this is fine for C code but it is not fine for pure Python code. Did you try something like

sage: e = DisjointSet(5)
sage: e.find(2**30)

[gmou3]

It causes a segmentation fault indeed. What should be done?

[gmou3]

@videlec I think you will be satisfied with the last changes.

[tscrim]

I believe @videlec's point is that user-facing code should not segfault on bad input (of course, internal code should not either, but there we can sanitize/control the input). So I believe your changes should have addressed this.

[gmou3]

Yes, I think it is good now.

[videlec]

Looking at it a bit more, I don't quite understand the purpose of this PR. If you want something fast(er), why don't you use directly the OrbitPartition C class? The DisjointSet provides one extra level of indirection which is a lot of overhead.

[gmou3]

Looking at it a bit more, I don't quite understand the purpose of this PR. If you want something fast(er), why don't you use directly the OrbitPartition C class? The DisjointSet provides one extra level of indirection which is a lot of overhead.

To some extent this is a fair point. However, the DisjointSet provides some extra functionality - e.g., the handling of hashables. Without any compromise in input checking, this PR still optimizes (at least) the union and find methods of the DisjointSet_of_hashables class. These methods performed unnecessary checks by passing through the union and find of DisjointSet_of_integers.

Given that now I call OP_find and OP_union directly from the find and union of DisjointSet_of_hashables, I wonder whether the _find and _union of DisjointSet_of_integers are still useful. Maybe if someone was to find them useful they should directly use the OrbitPartition class instead?

[videlec]

Looking at it a bit more, I don't quite understand the purpose of this PR. If you want something fast(er), why don't you use directly the OrbitPartition C class? The DisjointSet provides one extra level of indirection which is a lot of overhead.

To some extent this is a fair point. However, the DisjointSet provides some extra functionality - e.g., the handling of hashables. Without any compromise in input checking, this PR still optimizes (at least) the union and find methods of the DisjointSet_of_hashables class. These methods performed unnecessary checks by passing through the union and find of DisjointSet_of_integers.

Indeed, that looks like a good optimization!

Given that now I call OP_find and OP_union directly from the find and union of DisjointSet_of_hashables, I wonder whether the _find and _union of DisjointSet_of_integers are still useful. Maybe if someone was to find them useful they should directly use the OrbitPartition class instead?

I would not bother keeping them.

[mantepse]

Given that now I call OP_find and OP_union directly from the find and union of DisjointSet_of_hashables, I wonder whether the _find and _union of DisjointSet_of_integers are still useful. Maybe if someone was to find them useful they should directly use the OrbitPartition class instead?

I would not bother keeping them.

Maybe you could add a note in the docstring of find and union that these are the methods to use if no checking is needed?

[gmou3]

@videlec Cool!

@mantepse:

Given that now I call OP_find and OP_union directly from the find and union of DisjointSet_of_hashables, I wonder whether the _find and _union of DisjointSet_of_integers are still useful. Maybe if someone was to find them useful they should directly use the OrbitPartition class instead?

I would not bother keeping them.

Maybe you could add a note in the docstring of find and union that these are the methods to use if no checking is needed?

I removed them and referred to OP_find and OP_join. One should be able to use those directly and almost as easily.

[videlec]

Looks good to me. Thanks.

[tscrim]

I will take @videlec's comment to mean a positive review.