Skip to content

Conversation

mantepse
Copy link
Contributor

fix #40935 for connected_components

as with #40939: we have to decide whether this does not pollute the name space too much.

Copy link

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

@mantepse
Copy link
Contributor Author

mantepse commented Oct 1, 2025

Maybe this should really be n_components. @dcoudert, is there a good reason to include connected in the name?

(cf #40917, but also consider W.number_of_irreducible_components)

I just see num_components in knots/knotinfo.py and number_of_components in knots/link.py.

@dcoudert
Copy link
Contributor

dcoudert commented Oct 1, 2025

it is important to keep components to distinguish between connected, biconnected, triconnected etc. components.

@dcoudert
Copy link
Contributor

dcoudert commented Oct 1, 2025

a serious drawback of this change is that it makes it less intuitive for users to get the list of methods related to connected components. Currently, g.connected_component<TAB> gives a list of methods. So we should at least keep the aliases.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 1, 2025

There is absolutely no question whether we should keep the old names. They must stay, in my opinion.

The only question is whether adding aliases is worth it.

@dcoudert
Copy link
Contributor

dcoudert commented Oct 1, 2025

I'm not sure of the added value of this change. it does not help unifying the name of similar methods for biconnected or triconnected components. The name connected_components_number is quite clear I think.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 1, 2025

Yes, the only issue is consistency with the rest of sage, where we try to promote n_xxx or number_of_xxx now. Currently biconnected or triconnected does not appear in the sage codebase.

@dcoudert
Copy link
Contributor

dcoudert commented Oct 1, 2025

I understand the consistency issue.

For bi/tricconnected components, we are missing some methods. I will propose a PR for the number of bi/tri connected components.

@mantepse mantepse mentioned this pull request Oct 1, 2025
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.

Ok, let's go. LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
sagemathgh-40943: provide alias number_of_connected_components
    
fix sagemath#40935 for `connected_components`

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40943
Reported by: Martin Rubey
Reviewer(s): David Coudert
@vbraun vbraun merged commit f7a8f20 into sagemath:develop Oct 6, 2025
28 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.

harmonize xxx_number
3 participants