n_nodes and number_of_nodes_xxx in tree related classes #40939

mantepse · 2025-09-30T19:20:31Z

fix #40935 in tree related classes.

Should number_of_nodes also be an alias, or perhaps instead of n_nodes?
Should n_nodes_to_the_right, n_nodes_at_depth also be aliases?

Does this pollute the namespace too much?

github-actions · 2025-09-30T20:29:32Z

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

sagemathgh-40940: provide aliases for number_of_inversions and number_of_negative_ones fix sagemath#40935 in ASM as with sagemath#40939: we have to decide whether this does not pollute the name space too much. URL: sagemath#40940 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton

sagemathgh-40941: provide alias number_of_longest_increasing_subsequences fix sagemath#40935 in permutation as with sagemath#40939: we have to decide whether this does not pollute the name space too much. URL: sagemath#40941 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton

sagemathgh-40942: provide an alias number_of_relations fix sagemath#40935 in posets as with sagemath#40939: we have to decide whether this does not pollute the name space too much. URL: sagemath#40942 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton

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

sagemathgh-40951: add methods for biconnected components This PR do the following: - add method `number_of_biconnected_components` to follow the proposal of sagemath#40939 - add method `biconnected_components` - move method `is_biconnected` from `graph.py` to `connectivity.pyx` and expose it in `generic_graph.pyx` The addition of method `biconnected_component_containing_vertex` is more involved and deserves its own PR. Indeed, a cut vertex belongs to multiple biconnected components. ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#40951 Reported by: David Coudert Reviewer(s): Martin Rubey

sagemathgh-40940: provide aliases for number_of_inversions and number_of_negative_ones fix sagemath#40935 in ASM as with sagemath#40939: we have to decide whether this does not pollute the name space too much. URL: sagemath#40940 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton

sagemathgh-40941: provide alias number_of_longest_increasing_subsequences fix sagemath#40935 in permutation as with sagemath#40939: we have to decide whether this does not pollute the name space too much. URL: sagemath#40941 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton

sagemathgh-40942: provide an alias number_of_relations fix sagemath#40935 in posets as with sagemath#40939: we have to decide whether this does not pollute the name space too much. URL: sagemath#40942 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton

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

sagemathgh-40951: add methods for biconnected components This PR do the following: - add method `number_of_biconnected_components` to follow the proposal of sagemath#40939 - add method `biconnected_components` - move method `is_biconnected` from `graph.py` to `connectivity.pyx` and expose it in `generic_graph.pyx` The addition of method `biconnected_component_containing_vertex` is more involved and deserves its own PR. Indeed, a cut vertex belongs to multiple biconnected components. ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#40951 Reported by: David Coudert Reviewer(s): Martin Rubey

mantepse · 2025-10-07T08:11:54Z

This is the last ticket concerning the transition to n_xxx or number_of_xxx. It would be good to finish this before the next release, for consistency.

fchapoton · 2025-10-07T08:34:34Z

I am not happy with these changes. They make the methods less consistent than they were.

mantepse · 2025-10-07T08:42:25Z

Let me also note the following idea (I think I asked about it already once, but I cannot find the place anymore). We can hide aliases from tab-completion (or rather: produce aliases hidden from tab-completion) as follows:

class AlternatingSignMatrix(...):
    ...
    def __init__(self, parent, asm):
        ...
        self._matrix = asm
        self.__setattr__('number_negative_ones', self.number_of_negative_ones)
        Element.__init__(self, parent)
  
    def __dir__(self):
        return [f for f in super().__dir__() if f != "number_negative_ones"]

I am quite sure that we could also make this more automatic by having a class decorator or something similar. Do you see any downsides, @fchapoton?

mantepse · 2025-10-11T11:05:40Z

I am not happy with these changes. They make the methods less consistent than they were.

Sorry, I overlooked your comment. Could you provide an alternative? I think that we have n_xxx or number_of_xxx now everywhere, except here, don't we?

mantepse · 2025-10-15T06:28:13Z

ping?

fchapoton · 2025-10-16T18:04:40Z

I would prefer, for local consistency, to have "n_nodes_etc` for all these methods.

mantepse · 2025-10-16T18:17:43Z

Would you also be OK with number_of_etc for all of them? This would be more consistent with the general scheme in combinat, and also with the fact that these are methods that actually have to do a computation, in contrast to, for example, Graph.n_vertices.

fchapoton · 2025-10-16T18:24:26Z

as you wish

mantepse · 2025-10-28T16:55:27Z

@fchapoton, I switched to number_of_nodes, this is more consistent with the rest of combinat. I think it would be good to get this (or an alternative) into the next release, because it is almost a universal convention (with some exceptions like nrows, ngens, nvars, ndigits, nbits, ncusps) that users can follow now.

mantepse · 2025-10-30T07:53:03Z

ping?

dcoudert

LGTM.

sagemathgh-40939: n_nodes and number_of_nodes_xxx in tree related classes fix sagemath#40935 in tree related classes. cf sagemath#40932 Should `number_of_nodes` also be an alias, or perhaps instead of `n_nodes`? Should `n_nodes_to_the_right`, `n_nodes_at_depth` also be aliases? Does this pollute the namespace too much? URL: sagemath#40939 Reported by: Martin Rubey Reviewer(s): David Coudert

mantepse added 2 commits September 30, 2025 21:15

use n_nodes and number_of_nodes_xxx in tree related classes

879f9ab

fix whitespace

db5657d

github-actions bot added the s: needs review label Sep 30, 2025

mantepse requested a review from fchapoton September 30, 2025 19:20

mantepse added s: needs info and removed s: needs review labels Sep 30, 2025

mantepse mentioned this pull request Oct 1, 2025

harmonize xxx_number #40935

Closed

dcoudert mentioned this pull request Oct 1, 2025

add methods for biconnected components #40951

Merged

5 tasks

mantepse added s: needs review and removed s: needs info labels Oct 11, 2025

Merge branch 'sagemath:develop' into harmonize_number_methods_trees

e9e5965

mantepse added 2 commits October 28, 2025 08:52

Merge branch 'develop' into harmonize_number_methods_trees

439c286

switch from n_nodes to number_of_nodes for consistency

542ad45

mantepse mentioned this pull request Nov 9, 2025

replace spanning_trees_count with number_of_spanning_trees for consistency #41155

Merged

dcoudert approved these changes Nov 9, 2025

View reviewed changes

dcoudert added s: positive review and removed s: needs review labels Nov 9, 2025

vbraun merged commit 4c5defe into sagemath:develop Nov 11, 2025
24 of 25 checks passed

github-actions bot removed the s: positive review label Nov 11, 2025

Uh oh!

n_nodes and number_of_nodes_xxx in tree related classes #40939

n_nodes and number_of_nodes_xxx in tree related classes #40939

Uh oh!

Conversation

mantepse commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mantepse commented Oct 7, 2025

Uh oh!

fchapoton commented Oct 7, 2025

Uh oh!

mantepse commented Oct 7, 2025

Uh oh!

mantepse commented Oct 11, 2025

Uh oh!

mantepse commented Oct 15, 2025

Uh oh!

fchapoton commented Oct 16, 2025

Uh oh!

mantepse commented Oct 16, 2025

Uh oh!

fchapoton commented Oct 16, 2025

Uh oh!

mantepse commented Oct 28, 2025

Uh oh!

mantepse commented Oct 30, 2025

Uh oh!

dcoudert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mantepse commented Sep 30, 2025 •

edited

Loading

github-actions bot commented Sep 30, 2025 •

edited

Loading