Skip to content

Fix "NotImplementedError: an immutable graph does not change name" #40110

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

Merged

Conversation

Ordoviz
Copy link
Contributor

@Ordoviz Ordoviz commented May 15, 2025

Certain methods related to TSP (Travelling salesman problem) fail on immutable graphs:

sage: Graph('EJ~w', immutable=True).longest_cycle().order()
NotImplementedError: an immutable graph does not change name

sage: graphs.ChvatalGraph().copy(immutable=True).is_hamiltonian()
NotImplementedError: an immutable graph does not change name

This PR fixes this by setting the _name attribute directly instead of setting it via name().

Another fix would be to allow changing the name of immutable graphs by removing these two lines from name():

if self.is_immutable():
    raise NotImplementedError("an immutable graph does not change name")

This check was introduced in #15681 and has bitten us in the past. While it seems counterintuitive for immutable graphs to have mutable attributes, the _embedding attribute can already be mutated via set_embedding().

📝 Checklist

  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

@Ordoviz
Copy link
Contributor Author

Ordoviz commented May 15, 2025

The failing CI macos tests can be ignored (are unrelated to this PR).

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.

It is surprising that this issue has not been raised before since we already have many tests for immutable graphs.
FYI, I have started to add parameter immutable to many methods creating/modifying graphs (see e.g., #39177, #39755). This is a long road and help reviewing PRs is more than welcome.

The proposed fix is ok. It might be improved in the future.

LGTM.

@vbraun vbraun merged commit 4081fe3 into sagemath:develop May 18, 2025
11 of 20 checks passed
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.

3 participants