sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 471 forks source link

More trouble with immutable graphs #19973

Closed jaanos closed 8 years ago

jaanos commented 8 years ago

Here is a fix to a few cases where things fail with immutable graphs due to subgraph preserving the mutability of the graph. The affected methods are traveling_salesman_problem (and then also is_hamiltonian and hamiltonian_cycle) and is_clique with the parameter directed_clique=True.

These problems have either been missed, or, more likely, introduced in #19526.

CC: @nathanncohen

Component: graph theory

Keywords: graphs digraphs immutable

Author: Janoš Vidali

Branch: 97ab2e1

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/19973

jaanos commented 8 years ago

Branch: u/jaanos/more_trouble_with_immutable_graphs

jaanos commented 8 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
+Here is a fix to a few cases where things fail with immutable graphs due to `subgraph` preserving the mutability of the graph. The affected methods are `traveling_salesman_problem` (and then also `is_hamiltonian` and `hamiltonian_cycle`) and `is_clique` with the parameter `directed_clique=True`.

+These problems have either been missed, or, more likely, introduced in #19526.
jaanos commented 8 years ago

Changed keywords from none to graphs digraphs immutable

jaanos commented 8 years ago

Commit: 7cf72dd

jaanos commented 8 years ago

Author: Janoš Vidali

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:3

Wondering about the default: why shouldn't we use immutable = self.is_immutable()?

jaanos commented 8 years ago
comment:4

Hi!

This is the default for subgraph since #19526 (if that's what you meant). But in these cases the algorithms change these subgraphs (in fairly trivial ways, but still), so we need them to be mutable.

Janoš

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:5

Oh. My mistake: what I meant is that the functions will now return a mutable graph (e.g. the tsp) even when called on an immutable graph. Should we change the mutability status of those graphs we are about to return to match the status of self?

jaanos commented 8 years ago
comment:6

Yes, we could do that. Also, when calling is_hamiltonian, we don't really need to construct this subgraph at all (unless it's cached somewhere, but I don't see it in the current code).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 7cf72dd to 97ab2e1

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

97ab2e1Return an immutable cycle from traveling_salesman_problem if the input graph is immutable
jaanos commented 8 years ago
comment:8

OK, traveling_salesman_problem now preserves mutability. It still constructs the subgraph, even if called from is_hamiltonian - this is, after all, the easy part of the algorithm:)

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago

Reviewer: Nathann Cohen

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:9

OKayyy.. Thanks for this ticket, good to go!

vbraun commented 8 years ago

Changed branch from u/jaanos/more_trouble_with_immutable_graphs to 97ab2e1

jaanos commented 8 years ago
comment:12

Thanks again!

jaanos commented 8 years ago

Changed commit from 97ab2e1 to none