Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cythonize GraphicMatroid #37839

Merged
merged 12 commits into from
May 12, 2024
Merged

Conversation

gmou3
Copy link
Contributor

@gmou3 gmou3 commented Apr 20, 2024

A metamorphosis of graphic_matroid.py into graphic_matroid.pyx and graphic_matroid.pxd.
Things should be somewhat faster; test, e.g., the GraphicMatroid's _circuit function. An indirect and diluted test that shows improvement can be seen by running %time matroids.CompleteGraphic(8).circuits() (~half the time).

For the reviewer: The changes of this PR are only inside the matroids module. The rest come from #37835.

⌛ Dependencies

#37835: Deeper improvements of DisjointSet.

Copy link

github-actions bot commented Apr 20, 2024

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

Remove `_find` & `_union`.
Remove `DisjointSet_of_integers` `_d` attribute from `DisjointSet_of_hashables` (and adjust code).
Add notes recommending `OP_find` and `OP_union` if no input checking needed.
Touch-up docstrings.
@gmou3 gmou3 requested a review from tscrim April 29, 2024 09:59
vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
    
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 (sagemath#37839) I mention a test that confirms this.
    
URL: sagemath#37835
Reported by: gmou3
Reviewer(s): gmou3, Martin Rubey, Travis Scrimshaw, Vincent Delecroix
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to look at the C code generated in performance critical sections to see if there are additional places where type casting and other such declarations could simplify the C code (and hence, increase performance). Although I am basically okay to include this in the current state and do other general improvements in later PRs. It is up to you.

src/sage/matroids/graphic_matroid.pyx Show resolved Hide resolved
src/sage/matroids/graphic_matroid.pyx Outdated Show resolved Hide resolved
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@gmou3
Copy link
Contributor Author

gmou3 commented May 5, 2024

It might be good to look at the C code generated in performance critical sections to see if there are additional places where type casting and other such declarations could simplify the C code (and hence, increase performance). Although I am basically okay to include this in the current state and do other general improvements in later PRs. It is up to you.

I hadn't thought of doing that and I'll keep the idea in mind for future PRs.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then let's get this in. It seems like the tester failures are unrelated, but it would be good to have them be run... I don't know how to kick it to run it again.

@gmou3
Copy link
Contributor Author

gmou3 commented May 5, 2024

Please don't kick it as it may further break things.
There are many errors indeed; alas, what a state of disrepair!

@mkoeppe
Copy link
Member

mkoeppe commented May 8, 2024

I've repaired the CI, which complains:

sage -t --random-seed=109326302233194564031839857779385647036 src/sage/matroids/graphic_matroid.py
**********************************************************************
File "src/sage/matroids/graphic_matroid.py", line 490, in sage.matroids.graphic_matroid.GraphicMatroid._minor
Failed example:
    M._minor(deletions=frozenset([0,1,2]))
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 1146, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matroids.graphic_matroid.GraphicMatroid._minor[1]>", line 1, in <module>
        M._minor(deletions=frozenset([Integer(0),Integer(1),Integer(2)]))
      File "sage/matroids/graphic_matroid.pyx", line 462, in sage.matroids.graphic_matroid.GraphicMatroid._minor
        cpdef _minor(self, contractions, deletions):
    TypeError: _minor() takes exactly 2 positional arguments (0 given)
**********************************************************************
File "src/sage/matroids/graphic_matroid.py", line 492, in sage.matroids.graphic_matroid.GraphicMatroid._minor
Failed example:
    M._minor(contractions=frozenset([0,1,2]))
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 11[46](https://github.com/sagemath/sage/actions/runs/8959227503/job/24712775461?pr=37839#step:12:47), in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matroids.graphic_matroid.GraphicMatroid._minor[2]>", line 1, in <module>
        M._minor(contractions=frozenset([Integer(0),Integer(1),Integer(2)]))
      File "sage/matroids/graphic_matroid.pyx", line 462, in sage.matroids.graphic_matroid.GraphicMatroid._minor
        cpdef _minor(self, contractions, deletions):
    TypeError: _minor() takes exactly 2 positional arguments (1 given)
**********************************************************************

@tscrim
Copy link
Collaborator

tscrim commented May 9, 2024

Something is wrong with the CI. There is no more graphic_matroid.py file as it is changed to a .pyx file.

@mkoeppe
Copy link
Member

mkoeppe commented May 10, 2024

Indeed, sorry for the noise.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
    
A metamorphosis of `graphic_matroid.py` into `graphic_matroid.pyx` and
`graphic_matroid.pxd`.
Things should be somewhat faster; test, e.g., the `GraphicMatroid`'s
`_circuit` function. An indirect and diluted test that shows improvement
can be seen by running `%time matroids.CompleteGraphic(8).circuits()`
(~half the time).

`For the reviewer:` The changes of this PR are only inside the
`matroids` module. The rest come from sagemath#37835.

### ⌛ Dependencies

sagemath#37835: Deeper improvements of `DisjointSet`.
    
URL: sagemath#37839
Reported by: gmou3
Reviewer(s): gmou3, Kwankyu Lee, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
    
A metamorphosis of `graphic_matroid.py` into `graphic_matroid.pyx` and
`graphic_matroid.pxd`.
Things should be somewhat faster; test, e.g., the `GraphicMatroid`'s
`_circuit` function. An indirect and diluted test that shows improvement
can be seen by running `%time matroids.CompleteGraphic(8).circuits()`
(~half the time).

`For the reviewer:` The changes of this PR are only inside the
`matroids` module. The rest come from sagemath#37835.

### ⌛ Dependencies

sagemath#37835: Deeper improvements of `DisjointSet`.
    
URL: sagemath#37839
Reported by: gmou3
Reviewer(s): gmou3, Kwankyu Lee, Travis Scrimshaw
@vbraun vbraun merged commit 0e8fd93 into sagemath:develop May 12, 2024
19 of 22 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 2024
@gmou3 gmou3 deleted the cythonize_graphicmatroid branch May 25, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants