Skip to content
New issue

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

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

Already on GitHub? # to your account

Salvage work on gammoids #39155

Merged
merged 7 commits into from
Jan 27, 2025
Merged

Salvage work on gammoids #39155

merged 7 commits into from
Jan 27, 2025

Conversation

gmou3
Copy link
Contributor

@gmou3 gmou3 commented Dec 18, 2024

Add matroid classes Gammoid and TransversalMatroid. Transversal matroids are also gammoids.

From the archived sagetrac-mirror repository (branches public/matroids/transversal_matroids-23536 and u/zgershkoff/gammoids).

Resolves #23536, resolves #23601, resolves #23628.

From archived `sagetrac-mirror` (branches `public/matroids/transversal_matroids-23536` and `u/zgershkoff/gammoids`).
Add unpickling method for `Gammoid`.
Copy link

github-actions bot commented Dec 18, 2024

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

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

Thanks. Math aside, the code looks good to me.

I believe the math was also double-checked by you and the original author.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 23, 2025
    
Add matroid classes `Gammoid` and `TransversalMatroid`. Transversal
matroids are also gammoids.

From the archived [sagetrac-
mirror](https://github.com/sagemath/sagetrac-mirror)  repository
(branches [public/matroids/transversal_matroids-
23536](https://github.com/sagemath/sagetrac-
mirror/tree/public/matroids/transversal_matroids-23536) and
[u/zgershkoff/gammoids](https://github.com/sagemath/sagetrac-
mirror/tree/u/zgershkoff/gammoids)).

Resolves sagemath#23536, resolves sagemath#23601, resolves sagemath#23628.
    
URL: sagemath#39155
Reported by: gmou3
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 25, 2025
    
Add matroid classes `Gammoid` and `TransversalMatroid`. Transversal
matroids are also gammoids.

From the archived [sagetrac-
mirror](https://github.com/sagemath/sagetrac-mirror)  repository
(branches [public/matroids/transversal_matroids-
23536](https://github.com/sagemath/sagetrac-
mirror/tree/public/matroids/transversal_matroids-23536) and
[u/zgershkoff/gammoids](https://github.com/sagemath/sagetrac-
mirror/tree/u/zgershkoff/gammoids)).

Resolves sagemath#23536, resolves sagemath#23601, resolves sagemath#23628.
    
URL: sagemath#39155
Reported by: gmou3
Reviewer(s): Kwankyu Lee
@vbraun vbraun merged commit 8612691 into sagemath:develop Jan 27, 2025
22 of 23 checks passed
@tobiasdiez
Copy link
Contributor

As @collares pointed out in #38749, some of the tests fail with Python 3.12: https://github.com/sagemath/sage/actions/runs/12979684201/job/36195724175

File "src/sage/matroids/transversal_matroid.pyx", line 701, in sage.matroids.transversal_matroid.TransversalMatroid.transversal_extension
Failed example:
    M2 = M.transversal_extension(element='d', newset=True)
Exception raised:
    Traceback (most recent call last):
      File "/Users/runner/work/sage/sage/src/sage/doctest/forker.py", line 728, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/runner/work/sage/sage/src/sage/doctest/forker.py", line 1152, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matroids.transversal_matroid.TransversalMatroid.transversal_extension[3]>", line 1, in <module>
        M2 = M.transversal_extension(element='d', newset=True)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/matroids/transversal_matroid.pyx", line 671, in sage.matroids.transversal_matroid.TransversalMatroid.transversal_extension
        cpdef transversal_extension(self, element=None, newset=False, sets=[]):
      File "sage/matroids/transversal_matroid.pyx", line 792, in sage.matroids.transversal_matroid.TransversalMatroid.transversal_extension
        return TransversalMatroid(new_sets, groundset, labels)
      File "sage/matroids/transversal_matroid.pyx", line 181, in sage.matroids.transversal_matroid.TransversalMatroid.__init__
        raise ValueError("set labels cannot be element labels")
    ValueError: set labels cannot be element labels

Can you please have a look and fix these? Thanks!

@collares
Copy link
Contributor

Quoting @dcoudert in #39266:

May be the error is due to the declaration cpdef transversal_extension(self, element=None, newset=False, sets=[]): in src/sage/matroids/transversal_matroid.pyx. Using None seems more appropriate.

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 29, 2025

Thanks for the reporting. I figured out that there was a bug when choosing a new label for the newly created set.
See #39405.

@gmou3 gmou3 deleted the gammoids branch January 29, 2025 00:17
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 30, 2025
An error appears on some ``Python`` implementations because of a bug in
``transversal_extension``.
The solution is to ensure that the label of `newset` is not `element`.

Reported to me in
sagemath#39155 (comment).

URL: sagemath#39405
Reported by: gmou3
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 31, 2025
An error appears on some ``Python`` implementations because of a bug in
``transversal_extension``.
The solution is to ensure that the label of `newset` is not `element`.

Reported to me in
sagemath#39155 (comment).

URL: sagemath#39405
Reported by: gmou3
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 3, 2025
    
An error appears on some ``Python`` implementations because of a bug in
``transversal_extension``.
The solution is to ensure that the label of `newset` is not `element`.

Reported to me in
sagemath#39155 (comment).
    
URL: sagemath#39405
Reported by: gmou3
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 4, 2025
    
An error appears on some ``Python`` implementations because of a bug in
``transversal_extension``.
The solution is to ensure that the label of `newset` is not `element`.

Reported to me in
sagemath#39155 (comment).
    
URL: sagemath#39405
Reported by: gmou3
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
    
An error appears on some ``Python`` implementations because of a bug in
``transversal_extension``.
The solution is to ensure that the label of `newset` is not `element`.

Reported to me in
sagemath#39155 (comment).
    
URL: sagemath#39405
Reported by: gmou3
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
    
An error appears on some ``Python`` implementations because of a bug in
``transversal_extension``.
The solution is to ensure that the label of `newset` is not `element`.

Reported to me in
sagemath#39155 (comment).
    
URL: sagemath#39405
Reported by: gmou3
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 10, 2025
    
An error appears on some ``Python`` implementations because of a bug in
``transversal_extension``.
The solution is to ensure that the label of `newset` is not `element`.

Reported to me in
sagemath#39155 (comment).
    
URL: sagemath#39405
Reported by: gmou3
Reviewer(s): Frédéric Chapoton
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add tranvsersal matroids to constructor and advanced.py Gammoids Transversal Matroids
5 participants