Skip to content

Conversation

fchapoton
Copy link
Contributor

This is trying to enhance the uniformity of some method names between LatticePolytope and Polyhedron.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

Copy link

github-actions bot commented Sep 23, 2025

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

@mantepse
Copy link
Contributor

I think it would be good to have a more general scheme thought through before fixing these.

For graphs, we have G.num_verts, for matrices we have m.nrows. I dislike both, (I would prefer number_of_XXX), but could we please fix one naming scheme?

Or does this PR mean we fix n_XXX?

@mantepse
Copy link
Contributor

One argument in favor of the nXXX naming scheme (without the underscore) is that it is what's used for matrices, which is probably hardest to change without upsetting very many users.

Should we ask on sage-devel? What are the options, realistically?

  1. change everything to one scheme, deprecating the others - if so,
  • nXXX (as in matrix.nrows, GammaH(33,[2]).ncusps, parent.ngens)
  • n_XXX (as in polyhedron.n_vertices)
  • num_XXX (as in graph.num_verts, design.num_points)
  • number_of_XXX (as in most of combinat)
  1. change everything to one scheme, keeping the others as alias
  2. something in between

@fchapoton
Copy link
Contributor Author

fchapoton commented Sep 23, 2025

I would advocate for just doing what I am doing now. This PR is only about local coherence, curing a very concrete pain. I do not think that we can achieve global coherence, but for sure we can think a little bit about it.

@nbruin
Copy link
Contributor

nbruin commented Sep 24, 2025

I don't think n_point reads better than npoints and since it varies from nrows (which is never going to change because it's used too much), I think it makes the situation worse by having even more variants floating around.
I think num_points is fine, but I would say you'd first have to introduce it as an alias on equal footing and keep npoints around as legacy. We can see later if it should be deprecated. Deprecation is a real cost because people have to update their code; in this case for no other reason than that someone prefers to change the naming scheme. Introducing aliases does not particularly incur a cost on users/code writers.

@fchapoton
Copy link
Contributor Author

my aim is very restricted : we have two implementations of polytopes, and they have incompatible names for the corresponding methods.. I have chosen to uniformize towards the most used names among the two possibilities

@fchapoton
Copy link
Contributor Author

current situation

Polyhedron LatticePolytope
integral_points_count npoints
n_vertices nvertices
n_facets nfacets
n_rays
n_lines
n_equations
n_inequalities
n_Hrepresentation
n_Vrepresentation

changes from my proposal

Polyhedron LatticePolytope
n_points n_points
n_vertices n_vertices
n_facets n_facets

and n_points on the left is just an alias

@nbruin
Copy link
Contributor

nbruin commented Sep 24, 2025

OK, in that case there is indeed something to conform with. I'd still advocate for not deprecating the npoints spelling to lower the burden for LatticePolytope code maintainers out there. That includes code that is published as part of articles and will be hard/impossible to change. We all know published code has a limited lifespan, but we I don't think we need to actively shorten it when that can be easily avoided.

@fchapoton
Copy link
Contributor Author

You have a point. But deprecating and removing also has benefits, apart from just cleaning : the TAB-completion becomes shorter and better.

@nbruin
Copy link
Contributor

nbruin commented Sep 24, 2025

Do you think you get a chance to tell and explain that to the people out there that have their code broken by a deprecation of a function name for what are ultimately aesthetic reasons? Do you think they'll agree with you?

I know from experience that having to amend previously working code is very frustrating. It happened on a large scale with Python 2 -> Python 3 due to the parentheses around print and it was not enjoyable. It's good to keep in mind when you're considering inflicting something similar.

@mantepse
Copy link
Contributor

In the case at hand, I suspect that there are not too many users / using projects affected. However, I admit that I don't know.

It might be nice to have a service, eg., a mailing list, that projects could describe to, which would warn of intended deprecations, so they could voice the inflicted cost.

@mantepse
Copy link
Contributor

In the case at hand, I suspect that there are not too many users / using projects affected. However, I admit that I don't know.

It might be nice to have a service, eg., a mailing list, that projects could describe to, which would warn of intended deprecations, so they could voice the inflicted cost.

I just noticed that there is also n_integral_points in ppl_lattice_polytope.py.

@fchapoton
Copy link
Contributor Author

I would not oppose if we decide not to deprecate. Concerning python3, I know what it took.

We may strive here to reach some coherence inside the geometry folder. Most methods are currently "n_*" there.

Besides the methods in lattice_polytope that I have changed, we also have to change

cone.py:    def nrays(self):
fan.py:    def ngenerating_cones(self):

@mantepse
Copy link
Contributor

I am strongly in favour of introducing the aliases n_xxx and number_of_xxx simultaneously, or, at least have an agreement that a PR to that effect would be reviewed in the same release cycle.

I would not oppose the very limited deprecation of npoints, nvertices, nfacets, nrays and ngenerating_cones currently proposed, but I don't mind if they say. Maybe @seblabbe knows whether this would affect a lot of code in the wild?

@fchapoton
Copy link
Contributor Author

now without deprecations

@mantepse
Copy link
Contributor

I'm setting this to positive, since this was done meanwhile for #40887, and I think it should be done simultaneously.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 29, 2025
sagemathgh-40875: better uniformity for polytope methods
    
This is trying to enhance the uniformity of some method names between
`LatticePolytope` and `Polyhedron`.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#40875
Reported by: Frédéric Chapoton
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 29, 2025
sagemathgh-40887: convert some methods in designs and graphs to n_*
    
This replaces methods `num_points` and `num_blocks` by `n_points` and
`n_blocks` in designs.

Old names are kept as aliases.

cf also sagemath#40875

This is just a small reduction of the number of method with names
`num_*`

### 📝 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.
    
URL: sagemath#40887
Reported by: Frédéric Chapoton
Reviewer(s): Vincent Macri
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 30, 2025
sagemathgh-40875: better uniformity for polytope methods
    
This is trying to enhance the uniformity of some method names between
`LatticePolytope` and `Polyhedron`.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#40875
Reported by: Frédéric Chapoton
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 30, 2025
sagemathgh-40887: convert some methods in designs and graphs to n_*
    
This replaces methods `num_points` and `num_blocks` by `n_points` and
`n_blocks` in designs.

Old names are kept as aliases.

cf also sagemath#40875

This is just a small reduction of the number of method with names
`num_*`

### 📝 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.
    
URL: sagemath#40887
Reported by: Frédéric Chapoton
Reviewer(s): Vincent Macri
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