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

Implemented methods concerning bricks and braces of a matching covered graph #39065

Open
wants to merge 46 commits into
base: develop
Choose a base branch
from

Conversation

janmenjayap
Copy link
Contributor

@janmenjayap janmenjayap commented Dec 1, 2024

The objective of this issue is to implement the methods pertaining to the bricks and brace of a matching covered graph.

More specifically, this PR aims to implement the following two methods:

  • is_brace() | Check if the (matching covered) graph is a brace
  • is_brick() | Check if the (matching covered) graph is a brick.

This PR shall address the methods related to bricks, braces and tight cut decomposition of matching covered graphs.

Fixes #38216.
Note that this issue fixes a small part of the mentioned issue.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

This PR depends on the PR #38742 and #38892.

cc: @dcoudert.

Copy link

github-actions bot commented Dec 1, 2024

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

src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

Check your code to avoid redundant operations.

src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
@janmenjayap
Copy link
Contributor Author

Please note that for the following doctest:

One may set the ``coNP_certificate`` to be ``True``::

            sage: H = graphs.HexahedralGraph()
            sage: G = MatchingCoveredGraph(H)
            sage: G.is_brace(coNP_certificate=True)
            (True, None, None)
            sage: C = graphs.CycleGraph(6)
            sage: D = MatchingCoveredGraph(C)
            sage: D.is_brace(coNP_certificate=True)
            (False, [(0, 5, None), (2, 3, None)], {0, 1, 2})

It is passing in my local for the following two commands:

  1. ./sage -t src/sage/graphs/matching_covered_graph.py
  2. ./sage -t --long --warn-long 30.0 --random-seed=48323011685507437272567463462221430408 src/sage/graphs/matching_covered_graph.py

However, it is failing in the check for Build & Test / test-new (pull_request).

@dcoudert
Copy link
Contributor

dcoudert commented Dec 1, 2024

is the returned certificate valid ? if so, you must make the method or doctest more robust. This might be a side effect of some data structure (set, graph, etc.) that do not guaranty the order of the items.

@janmenjayap
Copy link
Contributor Author

janmenjayap commented Dec 1, 2024 via email

@user202729
Copy link
Contributor

Please note that for the following doctest:

One may set the ``coNP_certificate`` to be ``True``::

            sage: H = graphs.HexahedralGraph()
            sage: G = MatchingCoveredGraph(H)
            sage: G.is_brace(coNP_certificate=True)
            (True, None, None)
            sage: C = graphs.CycleGraph(6)
            sage: D = MatchingCoveredGraph(C)
            sage: D.is_brace(coNP_certificate=True)
            (False, [(0, 5, None), (2, 3, None)], {0, 1, 2})

It is passing in my local for the following two commands:

  1. ./sage -t src/sage/graphs/matching_covered_graph.py
  2. ./sage -t --long --warn-long 30.0 --random-seed=48323011685507437272567463462221430408 src/sage/graphs/matching_covered_graph.py

However, it is failing in the check for Build & Test / test-new (pull_request).

I don't think it's a very large issue. We have # random for that.

You can then do e.g. manual checking that the result is in some finite list in a TESTS:: block later.

@dcoudert
Copy link
Contributor

dcoudert commented Dec 7, 2024

You may implement a method to check that the returned certificate is valid and use it instead of printing the certificate.

src/sage/graphs/matching_covered_graph.py Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Outdated Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Show resolved Hide resolved
src/sage/graphs/matching_covered_graph.py Show resolved Hide resolved
@dcoudert
Copy link
Contributor

something goes wrong with lint.

@janmenjayap
Copy link
Contributor Author

Please note that I have added additional information in the output of is_brick() and is_brace() (without any extra calculation/ computation) that will be used in other methods (for instance bricks_and_braces() or tight_cut_decomposition()).

Also, (I just realised through some examples and testings that) the implementation of bricks_and_braces() requires the method merge_vertices() to be overwritten. So, it will be great if we could discuss regarding bricks_and_braces() and tight_cut_decomposition() in a separate PR as suggested earlier.

Please let me know if this PR requires any further modifications.

Thank you.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

Although you have already put (too) many tests, some parts of the code are not covered by tests. See codecov reports. Please add the missing tests.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

LGTM.
It seems difficult to fully satisfy codecov anyway.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 19, 2025
… a matching covered graph

    
<!-- ^ Please provide a concise and informative title. -->
The objective of this issue is to implement the methods pertaining to
the bricks and brace of a matching covered graph.

<!-- ^ 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. -->
More specifically, this PR aims to implement the following two methods:
<!-- - [ ] `bricks_and_braces()` | Return the list of (underlying simple
graph of) the bricks and braces of the (matching covered) graph. -->
- [x]  `is_brace()` | Check if the (matching covered) graph is a brace
- [x] `is_brick()` | Check if the (matching covered) graph is a brick.
<!-- - [ ] `number_of_braces()` | Return the number of braces. -->
<!-- - [ ] `number_of_bricks()` | Return the number of bricks. -->
<!-- - [ ] `number_of_petersen_bricks()` | Return the number of Petersen
bricks. -->
<!-- - [ ] `tight_cut_decomposition()` | Return a tight cut
decomposition. -->

<!-- v Why is this change required? What problem does it solve? -->
This PR shall address the methods related to bricks, braces and tight
cut decomposition of matching covered graphs.

<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
Fixes sagemath#38216.
Note that this issue fixes a small part of the mentioned issue.

### 📝 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.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies
This PR depends on the PR sagemath#38742 and sagemath#38892.

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

cc: @dcoudert.
    
URL: sagemath#39065
Reported by: Janmenjaya Panda
Reviewer(s): David Coudert, Janmenjaya Panda, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 20, 2025
… a matching covered graph

    
<!-- ^ Please provide a concise and informative title. -->
The objective of this issue is to implement the methods pertaining to
the bricks and brace of a matching covered graph.

<!-- ^ 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. -->
More specifically, this PR aims to implement the following two methods:
<!-- - [ ] `bricks_and_braces()` | Return the list of (underlying simple
graph of) the bricks and braces of the (matching covered) graph. -->
- [x]  `is_brace()` | Check if the (matching covered) graph is a brace
- [x] `is_brick()` | Check if the (matching covered) graph is a brick.
<!-- - [ ] `number_of_braces()` | Return the number of braces. -->
<!-- - [ ] `number_of_bricks()` | Return the number of bricks. -->
<!-- - [ ] `number_of_petersen_bricks()` | Return the number of Petersen
bricks. -->
<!-- - [ ] `tight_cut_decomposition()` | Return a tight cut
decomposition. -->

<!-- v Why is this change required? What problem does it solve? -->
This PR shall address the methods related to bricks, braces and tight
cut decomposition of matching covered graphs.

<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
Fixes sagemath#38216.
Note that this issue fixes a small part of the mentioned issue.

### 📝 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.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies
This PR depends on the PR sagemath#38742 and sagemath#38892.

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

cc: @dcoudert.
    
URL: sagemath#39065
Reported by: Janmenjaya Panda
Reviewer(s): David Coudert, Janmenjaya Panda, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 23, 2025
… a matching covered graph

    
<!-- ^ Please provide a concise and informative title. -->
The objective of this issue is to implement the methods pertaining to
the bricks and brace of a matching covered graph.

<!-- ^ 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. -->
More specifically, this PR aims to implement the following two methods:
<!-- - [ ] `bricks_and_braces()` | Return the list of (underlying simple
graph of) the bricks and braces of the (matching covered) graph. -->
- [x]  `is_brace()` | Check if the (matching covered) graph is a brace
- [x] `is_brick()` | Check if the (matching covered) graph is a brick.
<!-- - [ ] `number_of_braces()` | Return the number of braces. -->
<!-- - [ ] `number_of_bricks()` | Return the number of bricks. -->
<!-- - [ ] `number_of_petersen_bricks()` | Return the number of Petersen
bricks. -->
<!-- - [ ] `tight_cut_decomposition()` | Return a tight cut
decomposition. -->

<!-- v Why is this change required? What problem does it solve? -->
This PR shall address the methods related to bricks, braces and tight
cut decomposition of matching covered graphs.

<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
Fixes sagemath#38216.
Note that this issue fixes a small part of the mentioned issue.

### 📝 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.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies
This PR depends on the PR sagemath#38742 and sagemath#38892.

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

cc: @dcoudert.
    
URL: sagemath#39065
Reported by: Janmenjaya Panda
Reviewer(s): David Coudert, Janmenjaya Panda, user202729
@dcoudert
Copy link
Contributor

Set it back to positive review after merge with last beta.

@janmenjayap
Copy link
Contributor Author

janmenjayap commented Jan 25, 2025

Hi @dcoudert,
Let me add the relevant examples for those lines which have not been covered, one by one.

@dcoudert
Copy link
Contributor

ok

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.

On Decompositions, Generation Methods and related concepts in the theory of Matching Covered Graphs
3 participants