Skip to content

Conversation

Krishnadubey1008
Copy link
Contributor

This PR fixes #39756
Added a new clear() method in the BipartiteGraph class to clear the left and right sets.

Please review the changes and let me know if any further improvements needed.

📝 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

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.

This fixes the issue. Thanks.

It remains to comply with the coding style:

  • add an empty line before the method
  • add an empty line after Clear all the vertices and edges in the graph.
  • add an EXAMPLES block showing that it's working well. There is no need to repeat all the examples of method clear from generic graph.

@Krishnadubey1008
Copy link
Contributor Author

@dcoudert I had made the required changes to PR #39760 . Kindly review it!

"""
Clear all the vertices and edges in the graph.
This method will also clear the left and right vertex sets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like that would be more informative

This method extends the functionality of method :meth:`~sage.graphs.generic_graph.GenericGraph.clear` to also clear vertex sets `left` and `right`.

EXAMPLES::
sage: B = BipartiteGraph(graphs.CompleteBipartiteGraph(7, 9))
Copy link
Contributor

Choose a reason for hiding this comment

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

a smaller example like graphs.CycleGraph(4) or even a single edge is sufficient.

@Krishnadubey1008
Copy link
Contributor Author

@dcoudert I have incorporated the improvements, please tell me if further improvements are needed.

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.

ensure that comments are on 80 columns mode and use double ` so ``left``

@Krishnadubey1008
Copy link
Contributor Author

@dcoudert I had incorporated the changes

"""
Clear all the vertices and edges in the graph.
This method extends the functionality of method :meth:`~sage.graphs.generic_graph.GenericGraph.clear`
Copy link
Contributor

Choose a reason for hiding this comment

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

        This method extends the functionality of method
        :meth:`~sage.graphs.generic_graph.GenericGraph.clear`
        to also clear vertex sets ``left`` and ``right``.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

There is a misunderstanding: I was not asking for removing the one line description of the method. Please restore Clear all the vertices and edges in the graph.

@Krishnadubey1008
Copy link
Contributor Author

@dcoudert done

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.

Tested on my laptop as I cannot launch the CIs.

@user202729
Copy link
Contributor

While this fixes the immediate issue, it doesn't fix the underlying issue. In particular delete_vertices remains broken.

sage: B = BipartiteGraph([(0,2), (0,3), (1,2), (1,3)])
sage: B
Bipartite graph on 4 vertices
sage: B.delete_vertices(B.vertex_iterator())
sage: B
Bipartite graph on 0 vertices
sage: B.left
{0, 1}

The reason is delete_vertices iterates through the input twice, which doesn't deal with iterator input well.

Once delete_vertices is fixed, this newly added method would be redundant. So I don't think it's a particularly good idea to have this in now.

@dcoudert
Copy link
Contributor

If I understand well, you propose to close and forget this PR and open a new one to fix delete_vertices ?

@user202729
Copy link
Contributor

If I understand well, you propose to close and forget this PR and open a new one to fix delete_vertices ?

Yes.

@dcoudert
Copy link
Contributor

@Krishnadubey1008, are you able to do the requested new PR ?

@Krishnadubey1008
Copy link
Contributor Author

yes, i can try. should I open a new issue or try fix it in this PR itself

@dcoudert
Copy link
Contributor

open a new PR. The issue remains #39756

@Krishnadubey1008
Copy link
Contributor Author

@dcoudert I had opened a PR #39813 to fix delete_vertices please review.

@dcoudert
Copy link
Contributor

This PR can be closed. A better fix is proposed in #39813.

Thank you @Krishnadubey1008 for the work.

@dcoudert
Copy link
Contributor

@vbraun you can close this PR. A better fix is proposed in #40730. Thank you.

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.

Calling clear() on BipartiteGraph does not clear left and right vertex sets
3 participants