graphs: make init_short_digraph always sort neighbors but without the extra log complexity #38427
Conversation
|
Setting this PR to a draft: the doctests that I added to I have to:
Note that the sorting of vertices in |
|
Documentation preview for this PR (built with commit 8627957; changes) is ready! 🎉 |
|
See issue #38527 |
|
Failing doctest in |
|
In 71113cd I start to fix some tests whose output where changed by bd63dd7 (where sort is remove from First it is not an easy task as it touches lots of different fields (LieAlgebra, Poset, ...) that I am not familar with. But from what I understand, static graphs are used in these structures (for example the hasse_diagram attribute of Poset is a static digraph) and are the reason for the changes in the output. Second, I hate tests whose output depend on internal representation of objects... 😠 (for example when the output is a list where the order does not matter but depends on the internal representation of the classes Graph and DiGraph). When possible I added Special mention for the doctest of the method |
|
I fixed all tests except the two from This PR can now be reviewed. |
There was a problem hiding this comment.
This is a smart improvement. Thanks. I have only minor comments.
The tests in connectivity.pyx should be fixed since #38535 as been merged.
I agree with you that doctests should not depend on the internal representation of objects. We reduced the number of such doctests, but we still have some.
|
Changes are done. |
…rs but without the extra log complexity
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
This PR improve the `init_short_digraph` function that is used to
initialize `StaticSparseCGraph` (used for immutable `Graph` and
`DiGraph`).
Before, a boolean parameter `sort_neighbors` was used to specify if we
wanted to sort the neighbors or not. It implied an extra `log` in the
complexity (as `qsort` was called).
With this PR, the neighbors are always sorted at no extra cost. It is
done by appending to the neighbors list the vertices in the correct
order so the call to `qsort` is not needed anymore.
This PR partly reverts sagemath#38198 and mostly reverts sagemath#37662
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
URL: sagemath#38427
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
|
On 32-bit I'm getting |
|
@vbraun I really do not know what to do about these tests. Were they passing before the PR ? For example the first one in your list check the return value of a method |
|
the doctest in |
|
Test were passing before this PR If Together with sorting output this would probably fix most |
github-actions
bot
added
s: needs review
and removed
s: positive review
s: needs work
labels
done in 23494ba For the tests in "src/sage/categories/regular_crystals.py", line 841, I think this is due to a bug in the class EdgesView of graphs: the tests asks for the vertices to be sorted u, v, _ = ([[1, 3, 5], [2, 4]], [[1, 2, 5], [3, 4]], 2)
u < v # should be True
FalseCan someone confirm ? Edit: sage: H = Graph([[1, 'a'],[ (1, 'a')]])
sage: H.edges(sort_vertices=True)
[(1, 'a', None)]If sorting were really performed, an exception should have been raised. I am looking into the others. |
I disabled the two tests that depend on `an_element` method on 32-bit: one should not write tests depending on "random" element.
|
Commit 8627957 should fix the remaining tests. I used (*) the failing tests relies an the fact that an element returned by the For me the remaining problem is the failing doctest in "src/sage/categories/regular_crystals.py". But as I say earlier I suspect a bug in EdgesView. If it is confirm, I can open a ticket for this bug, but I will not try to fix it in this PR. |
in sage: sage: H = Graph([[0, 1, 2, 'a'],[ ('a', 1)]])
....: sage: H.edges(sort_vertices=True)
[('a', 1, None)]I don't know what to do here. |
|
I will close this PR and open a new one with less changes to avoid all the breaking doctests. Mainly I will remove commit bd63dd7 which removes 4 lines in StaticSparseBackend code that sort the vertices before creating the graph. |
|
I agree, it's best to have a PR dedicated to the change in StaticSparseBackend and in method |
…rs but without the extra log complexity (2nd try)
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
This PR is based on the closed PR sagemath#38427 (by myself) that was breaking
too much doctests.
This PR improve the `init_short_digraph` function that is used to
initialize `StaticSparseCGraph` (used for immutable `Graph` and
`DiGraph`).
Before, a boolean parameter `sort_neighbors` was used to specify if we
wanted to sort the neighbors or not. It implied an extra `log` in the
complexity (as `qsort` was called).
With this PR, the neighbors are always sorted at no extra cost. It is
done by appending to the neighbors list the vertices in the correct
order so the call to `qsort` is not needed anymore.
This PR partly reverts sagemath#38198 and mostly reverts sagemath#37662
Contrary to PR sagemath#38427, I did not include the patch that remove the
sorting of vertices when StaticSparseGraph are initialized because it
was breaking too many doctests. Instead I added a option to disabled
said sorting and use it in my doctests to check that the new code works.
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
URL: sagemath#38664
Reported by: cyrilbouvier
Reviewer(s): David Coudert
This PR improve the
init_short_digraphfunction that is used to initializeStaticSparseCGraph(used for immutableGraphandDiGraph).Before, a boolean parameter
sort_neighborswas used to specify if we wanted to sort the neighbors or not. It implied an extralogin the complexity (asqsortwas called).With this PR, the neighbors are always sorted at no extra cost. It is done by appending to the neighbors list the vertices in the correct order so the call to
qsortis not needed anymore.This PR partly reverts #38198 and mostly reverts #37662
📝 Checklist
⌛ Dependencies