Implementing is_matching_covered() and is_bicritical()

[janmenjayap]

The objective of this issue is to implement is_matching_covered() and is_bicritical() and to collect the matching related methods and to put them under matching.py.

This PR introduces two methods pertaining to matching and aims to list out all matching related functions in matching.py. The two new methods are described below:

• is_matching_covered(): Check if the graph is matching covered.
• is_bicritical(): Check if the graph is bicritical.

This PR addresses couple of missing functions and also addresses the lack of a separate file for matchings.

Fixes #38216.

📝 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

Nothing as of now (up to my knowledge).

cc: @dcoudert.

[github-actions]

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

[dcoudert]

This is the shape of the class. You now have to code and document it. In particular, don't forget to add in the description of the module what a matching covered graph is.

[janmenjayap]

Sure. Thanks for the feedback. 😊👍

I was wondering regarding the following:
Can the Undirected graphs module be modified as follows:

1. Putting matching_polynomial(), has_perfect_matching(), is_bicritical(), is_factor_critical(), is_matching_covered(), matching() and perfect_matchings() under a section named Matching, and
2. Renaming Leftovers to Miscellaneous.

Please correct me in case I missed something.

[dcoudert]

yes this can be done, but in another PR. I fear that you might create a giant PR that will be very hard to review.

[janmenjayap]

Also,
I have one more doubt, the Build & Test / test-new (pull_request) is failing majorly for two reasons:

1. For the following example under M_alternating_even_mark():

sage: # K(4) ☉ K(3, 3) is matching covered
sage: G = Graph()
sage: G.add_edges([
....:    (0, 2), (0, 3), (0, 4), (1, 2),
....:    (1, 3), (1, 4), (2, 5), (3, 6),
....:    (4, 7), (5, 6), (5, 7), (6, 7)
....: ])
sage: M = Graph(G.matching())
sage: G.is_matching_covered(M)
True
sage: u = 0
sage: v = next(M.neighbor_iterator(u))
sage: S = G.M_alternating_even_mark(u, M)
sage: (set(G.neighbor_iterator(v))).issubset(S)
True

it throws the following error:

TypeError: Graph.is_matching_covered() takes 1 positional argument but 2 were given

which shall be resolved once is_matching_covered() is implemented.

2. The following line in is_bicritical():

4994            components = self.connected_components()

for the following example:

sage: cycle1 = graphs.CycleGraph(4)
sage: cycle2 = graphs.CycleGraph(6)
sage: cycle2.relabel(lambda v: v + 4)
sage: G = Graph()
sage: G.add_edges(cycle1.edges() + cycle2.edges())
sage: len(G.connected_components())
2
sage: G.is_bicritical()
False

throws this for len(G.connected_components()):

DeprecationWarning: parameter 'sort' will be set to False by default in the future See https://github.com/sagemath/sage/issues/35889 for details. 
2

This shows up in the local testing as well, but since it gave correct output 2, I thought this should not be problem. Any comments on this?

[dcoudert]

TypeError: Graph.is_matching_covered() takes 1 positional argument but 2 were given:

Currently, method Graph.is_matching_covered is defined as def is_matching_covered(self):, so when you call G.is_matching_covered(M), G is self.

For the call to G.connected_components(), set parameter sort=False.

[dcoudert]

As I tried to explain earlier, I think that all methods related to matchings should be placed in a module called matching.py and then linked to Graph the same way we do for other methods. Indeed, I don't think that M_alternating_even_mark is needed in Graph. This method seems very specific and needed as a building block for other methods, no ?

[janmenjayap]

As I tried to explain earlier, I think that all methods related to matchings should be placed in a module called matching.py and then linked to Graph the same way we do for other methods. Indeed, I don't think that M_alternating_even_mark is needed in Graph. This method seems very specific and needed as a building block for other methods, no ?

Sure.
Along with that, another possibility can be making M_alternating_even_mark() private.

[grhkm21]

To fix "TypeError: Graph.is_matching_covered() takes 1 positional argument but 2 were given", you can make the argument list of is_matching_covered be def is_matching_covered(self, *_, **__):. Also do you mind replacing the pass with raise NotImplementedError()?

[dcoudert]

To fix "TypeError: Graph.is_matching_covered() takes 1 positional argument but 2 were given", you can make the argument list of is_matching_covered be def is_matching_covered(self, *_, **__):.

You may use this trick during the development period, but it should not stay for the final version.

[dcoudert]

I have a suggestion: Would it be possible to consider renaming the 'Leftovers' section in Undirected graphs module to 'Miscellaneous'?

Also, considering the last commit, the way git compared the old and new changes in the last commit seems a bit unusual, especially since the main change was moving some methods from graph.py to matching.py.

May be, but not in this PR. It is bad practice to do too many things in a single PR as it makes it harder to review.

[dcoudert]

5402 +            dfs(H, H.vertices()[0], visited_in, None, parent_in, 'in')

Could you please let me know how to use next(H.neighbor_iterator()) here? I suppose I should set the 0th vertex as root. Thank you.

Yes, it should be possible to replace H.vertices()[0] with next(H.neighbor_iterator()).

[janmenjayap]

5402 +            dfs(H, H.vertices()[0], visited_in, None, parent_in, 'in')

Could you please let me know how to use next(H.neighbor_iterator()) here? I suppose I should set the 0th vertex as root. Thank you.

Yes, it should be possible to replace H.vertices()[0] with next(H.neighbor_iterator()).

        root = next(H.neighbor_iterator())

        visited_in = {v: False for v in H}
        parent_in = {v: None for v in H}
        dfs(H, root, visited_in, None, parent_in, 'in')

        visited_out = {v: False for v in H}
        parent_out = {v: None for v in H}
        dfs(H, root, visited_out, None, parent_out, 'out')

In the above piece of code, I got the following error:

TypeError: GenericGraph.neighbor_iterator() missing 1 required positional argument: 'vertex'

[dcoudert]

        root = next(H.neighbor_iterator())

In the above piece of code, I got the following error:

TypeError: GenericGraph.neighbor_iterator() missing 1 required positional argument: 'vertex'

arghh, of course, it's not neighbor_iterator but vertex_iterator !!!

[dcoudert]

some tests are failing.

[dcoudert]

one remaining minor comment. Otherwise LGTM.

[janmenjayap]

It seems like there are other files for instance /sage/src/sage/doctest/forker.py that import methods like has_perfect_matching() from graph.py. That's the reason after the removal of lazy_import corresponding these methods some tests are failing.

Shall we add them back in this PR itself?

[dcoudert]

For each method moved from graph.py to matching.py, you must expose the method in graph.py, as done for other methods at the bottom of the file.

I only doubt that exposing M_alternating_even_mark in Graph is useful. The method is too specific and can be accessed if needed anyway.

[janmenjayap]

For each method moved from graph.py to matching.py, you must expose the method in graph.py, as done for other methods at the bottom of the file.

I only doubt that exposing M_alternating_even_mark in Graph is useful. The method is too specific and can be accessed if needed anyway.

I suppose the inclusion of M_alternating_even_mark is required for the successful completion of the test cases and examples of the same method as the method is used as an attribute of Graph class in those tests and examples.

[dcoudert]

I suppose the inclusion of M_alternating_even_mark is required for the successful completion of the test cases and examples of the same method as the method is used as an attribute of Graph class in those tests and examples.

Or change calls from G_simple.M_alternating_even_mark(u, M) to M_alternating_even_mark(G_simple, u, M)...

[dcoudert]

This looks good to me.