-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
some care in sage/graphs/domination.py #35982
Conversation
@@ -730,7 +737,7 @@ def _aux_with_rep(H, to_dom, u_next): | |||
break | |||
|
|||
|
|||
def minimal_dominating_sets(G, to_dominate=None, work_on_copy=False, k=1): | |||
def minimal_dominating_sets(G, to_dominate=None, work_on_copy=True, k=1): | |||
r""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a deprecation period for this change of default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a deprecation, but here I consider that we fix a bug. The default value of parameter work_on_copy
was not consistent with the code: the graph was modified when set to True
and a copy was created when set to False
.
Furthermore, I think it is an error to modify the input graph without any explicitly action from the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes sense.
Thank you for the review. |
Documentation preview for this PR (built with commit 945e476; changes) is ready! 🎉 |
We do some minor changes in `sage/graphs/domination.py`. The main change is the default value of parameter `work_on_copy` from `False` to `True`. Firstly, it is safer to ensure that the input graph is not modified by default, unless the user asks for such behavior. Secondly, the method was actually working on a copy when the parameter was `False` (wrong order of tests). We also add some tests to show that the methods are robust to vertices with incomparable labels (see sagemath#35902). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#35982 Reported by: David Coudert Reviewer(s): David Coudert, Matthias Köppe
We do some minor changes in
sage/graphs/domination.py
.The main change is the default value of parameter
work_on_copy
fromFalse
toTrue
. Firstly, it is safer to ensure that the input graph is not modified by default, unless the user asks for such behavior. Secondly, the method was actually working on a copy when the parameter wasFalse
(wrong order of tests).We also add some tests to show that the methods are robust to vertices with incomparable labels (see #35902).
📝 Checklist
⌛ Dependencies