Consistent naming for factory methods

[Michael Orlitzky]

An official bikeshed thread!

We have a number of grouped factory methods to construct familiar
objects. Some of them use CamelCase for their method names:

sage: graphs.<tab>
sage: manifolds.<tab>
sage: valuations.<tab>

Some use underscores for their method names:

sage: polytopes.<tab>

Some can't make up their minds:

sage: designs.<tab>
sage: toric_varieties.<tab>

To confound matters, when an object has an "official" name in either
upper- or lower-case, we tend to use that rather than follow a
convention in the first place.

In ticket #26623, I need to pick one for the "cones" object.

PEP8 says that, since these are method names, they should use
underscores. And the sage developer guide says to follow PEP8:

http://doc.sagemath.org/html/en/developer/coding_basics.html

But it also says

Use CamelCase for class names... and factory functions that mimic
object constructors, for example PolynomialRing.

That makes complete sense for top-level functions like PolynomialRing.
Should it apply to *methods* that also act like constructors, even
though visually, something like cones.Foo() doesn't look like a
constructor (it looks like a method)?

Can we pick one style and write it down to avoid this problem in the future?

FWIW, I think cones.trivial() looks better than cones.Trivial(), but for
no good reason. I think it's just because I haven't seen method names
capitalized in a long time.

[Vincent Delecroix]

To my mind, it does not hurt to have these `factory.X` as CamelCase
or snake_case. The reason is that the factory should be thought
of as a Python module (which contains both objects and functions). I
agree that it would be nice to uniformize that a bit (and update the
developer guide once we have reasonable guidelines).

Note that

sage: groups.<TAB>

which is furthermore refined in

sage: groups.affine.<TAB>
sage: groups.lie.<TAB>
etc

also uses CamelCase.

[Kwankyu Lee]

On Friday, February 14, 2020 at 11:01:05 PM UTC+9, Michael Orlitzky wrote:

Should it apply to *methods* that also act like constructors, even
though visually, something like cones.Foo() doesn't look like a
constructor (it looks like a method)?

 

I would think

cones                            as a module or a package,

cones.Foo()                  as a constructor (class or factory that mimic class),

cones.do_something()  as a function.

Perhaps we also need an official name for things like "cones". A constructor group?

Can we pick one style and write it down to avoid this problem in the future?

+1 

FWIW, I think cones.trivial() looks better than cones.Trivial(), but for
no good reason. 

+1 to cones.trivial()

reason: there is no Trivial class or factory. 

[Michael Orlitzky]

On 2/14/20 4:53 PM, Kwankyu Lee wrote:
>
> +1 to cones.trivial()
>
> reason: there is no Trivial class or factory. 
>

I guess this comes down to how strict your definition of a factory
method is. Traditionally, the factory pattern meant that you would ask
for an object with certain properties and get back a subclass that met
your needs. Indeed, with PolynomialRing, you can get back different
classes, e.g.

sage: PolynomialRing(SR,'x').__class__
sage: PolynomialRing(QQ,'x,y').__class__

With cones.Trivial(..), you always get back the *same* class, namely a
ConvexRationalPolyhedralCone. So that is not a factory in the same sense.

From another perspective, if I didn't want to group these all under the
"cones" prefix, what would I do? If I made them all subclasses of
ConvexRationalPolyhedralCone, then there would be a class named Trivial
and Trivial(..) would be used to construct one. But that's not what I'd
do in this case, because I don't want to return a subclass -- I want to
return a particular ConvexRationalPolyhedralCone. Thus I would have a
top-level function named trivial() do the work, and if you wanted to
preface it with its module name, it would look like cones.trivial().

But... is that the distinction that we want to make? I'm skeptical that
the person who wrote that paragraph in the developer's guide had the
1995 definition of "factory" in mind. I actually like this convention,
but it's a bit subtle.

[Michael Orlitzky]

On 2/14/20 7:55 PM, Michael Orlitzky wrote:
>
> But... is that the distinction that we want to make? I'm skeptical that
> the person who wrote that paragraph in the developer's guide had the
> 1995 definition of "factory" in mind. I actually like this convention,
> but it's a bit subtle.
>

...and confusing as hell to users, since it's based on an implementation
detail. I tried to write up a paragraph of documentation for this
convention and that was enough to convince me it's a bad idea.

Ultimately, these functions/methods are grouped for the convenience of
users, who want to type something like matrix.i<tab> and not have it
fail because the thing they're looking for is named matrix.Identity().
As a result, I think we should pick one, and stick with it -- either
PEP8, or CamelCase.

For the same reason, I am becoming more convinced that lowercase with
underscores is the way to go. Even if you can think of some of these as
being thin wrappers around constructors or as factory functions, the
reality is that users won't and can't know that. It looks like a method,
and the first thing they're going to try if they're looking for the
nonnegative orthant is cones.n<tab>, because that's how thing.<tab>
works in most other situations.

[Kwankyu Lee]

On Sunday, February 16, 2020 at 10:10:11 AM UTC+9, Michael Orlitzky wrote:

On 2/14/20 7:55 PM, Michael Orlitzky wrote:
>
> But... is that the distinction that we want to make? I'm skeptical that
> the person who wrote that paragraph in the developer's guide had the
> 1995 definition of "factory" in mind. I actually like this convention,
> but it's a bit subtle.
>

...and confusing as hell to users, since it's based on an implementation
detail. I tried to write up a paragraph of documentation for this
convention and that was enough to convince me it's a bad idea. 

It would also be mildly confusing to allow both of these

    from sage.coding.punctured_code import PuncturedCode

    PuncturedCode(...)

and 

    codes.punctured_code(...)

So your suggestion would imply that we should discourage users from using the former style. Then that we don't have

R.<x> = rings.polynomial_ring(QQ)

but only

R.<x> = PolynomialRing(QQ)

would confuse some users.

 

[Michael Orlitzky]

On 2/15/20 9:33 PM, Kwankyu Lee wrote:
>
> It would also be mildly confusing to allow both of these
>
>     from sage.coding.punctured_code import PuncturedCode
>     PuncturedCode(...)
>
> and 
>
>     codes.punctured_code(...)
>
> So your suggestion would imply that we should discourage users from
> using the former style. Then that we don't have
>
> R.<x> = rings.polynomial_ring(QQ)
>
> but only
>
> R.<x> = PolynomialRing(QQ)
>
> would confuse some users.
>

I wrote a rather long response to this about the design decisions that
go into a programming language versus those that go into a discoverable
end-user interface, and about how we have to live with some confusion
because of the python ClassName convention... but then I realized that
PEP8 already says you can use lowercase for class names:

Class names should normally use the CapWords convention. The naming
convention for functions may be used instead in cases where the
interface is documented and used primarily as a callable.

- https://www.python.org/dev/peps/pep-0008/#class-names

So yes, let me internally adopt the more radical stance =)

In the future, classes like FiniteDimensionalAlgebra, whose constructors
are the primary (documented!) user-interface to that class, should use
the lowercase-with-underscores function naming scheme. Since it's legal
in python, we should do it so that our users don't have to guess whether
or not the first letter of the thing they want is capitalized.

Obviously this would take forever to gain traction (and would face a lot
more resistance), but it's a nice thought. In the meantime, regardless
of whether or not anyone agrees with that for true constructors, I don't
think we should perpetuate the issue into settings like cones.<tab>
where there's no historical technical reason to do so.

[TB]

I think the name currently used in the docs for most of these factory
objects/modules is "catalog". Searching the docs [1] gives catalogs for
algebras, crystals, groups (with sub-catalogs), manifolds, codes,
designs, matroids, posets and others.

There are classes like SymmetricGroup, whose name is well-known and was
set a long time ago. This is also the spelling in GAP and Magma, FWIW.

One name that I think should be changed is CyclicPermutationGroup, to
CyclicGroup. If we can have groups.permutation.Symmetric and
groups.presentation.Symmetric without too much confusion, then cyclic
groups can follow the same naming convention.

Regards,
TB

[1]
https://doc.sagemath.org/html/en/reference/search.html?q=catalog&check_keywords=yes&area=default