Make base ring appear in self representation of PointConfiguration

[mo271]

When defining point configurations with other base ring than QQ, the self reprensentation of the instance of the class PointConfiguration still returns QQ.

sage: AA(sqrt(2))
1.414213562373095?
sage: PointConfiguration([[AA(sqrt(2))]])
A point configuration in QQ^1 consisting of 1 point. The triangulations of this point configuration are assumed to be connected, not necessarily fine, not necessarily regular.
sage: PointConfiguration([[AA(sqrt(2))]]).base_ring()
Algebraic Real Field

The hardcoded "QQ" should be changed to be more flexible and actually return the type of base ring; e.g. AA, RDF, RLF or whatever.

---

The code that needs to be changed reads:

        if self.is_affine():
            s += ' in QQ^'+str(self.ambient_dim())
        else:
            s += ' in P(QQ^'+str(self.ambient_dim()+1)+')'
        if len(self)==1:
            s += ' consisting of '+str(len(self))+' point. '
        else:
            s += ' consisting of '+str(len(self))+' points. '

Component: geometry

Keywords: base_ring

Author: Moritz Firsching

Branch/Commit: a11676b

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/22132

[nbruin]

comment:1

I think there are some issues with that class. First of all, the fact that it's "UniqueRepresentation" is quite awful: it means that the construction parameters are cached globally, and hence pinned in memory until the structure disappears.

Furthermore, is this actually a parent?

sage: P=PointConfiguration([[1.17,2]])
sage: parent(P[0])
<type 'sage.geometry.triangulation.base.Point'>

and the derivation of the base ring seems fishy too:

sage: PointConfiguration([[1.0,2]]).base_ring()
Integer Ring
sage: PointConfiguration([[1.1,2]]).base_ring()
Real Field with 53 bits of precision

That seems wrong.

For the string representation, I'd go for

"Point configuration in affine %s-space over %s"%(P.ambient_dim(),P.base_ring())

The reality is, QQ,AA,RDF aren't actually print names of rings. They are just convenience bindings to commonly occurring rings.

[mo271]

Branch: u/moritz/22132

[mo271]

Commit: b5ac409

[mo271]

comment:3

I agree that some other behavior is problematic in this class. I like your proposal for the string representation and prepared a branch with the necessary changes.

---

New commits:

b5ac409

changing self-representation of PointConfiguration (and all the doctests)

[tscrim]

comment:4

This is actually a parent; the problem is that the __getitem__ returns the points, not the elements:

sage: p = PointConfiguration([[0,0],[0,1],[1,0],[1,1],[-1,-1]])
sage: p.an_element()
(<1,3,4>, <2,3,4>)
sage: parent(p.an_element()) is p
True
sage: list(p)
[P(0, 0), P(0, 1), P(1, 0), P(1, 1), P(-1, -1)]

Although I don't see the problem with this being a UniqueRepresentation. It isn't like the inputs are some strange/large objects that we want garbage collected. I was going to say there is enough computationally heavy things cached to further justify this, but what gets cached is not what I was expecting.

[nbruin]

comment:5

Replying to @tscrim:

This is actually a parent; the problem is that the __getitem__ returns the points, not the elements:

sage: p = PointConfiguration([[0,0],[0,1],[1,0],[1,1],[-1,-1]])
sage: p.an_element()
(<1,3,4>, <2,3,4>)

OK, I see. The documentation indeed says that elements of a pointconfiguration are triangulations (not points as the name suggests).

Although I don't see the problem with this being a UniqueRepresentation. It isn't like the inputs are some strange/large objects that we want garbage collected.

That can be a lot of points! And as you can see (because the parent needs to be normalized), the original input points would be a rather bad key to base the construction on. Also, because the input is so extensive, I would expect it to be very rare that someone would be trying to construct the same configuration twice without having access to exactly the same list of points. So I think there is no benefit that is derived from caching the parent. There are definitely drawbacks in that by having a weak reference from a global dictionary pointing at an object, you can get obscure memory leaks quite easily. So the problem I see with UniqueRepresentation is that it's used without a clear benefit. I think it can safely be downgraded to EqualityById (which would still give you lightning fast hashing and equality testing).

I was going to say there is enough computationally heavy things cached to further justify this, but what gets cached is not what I was expecting.

Yes, the caching is fine, but I don't think we'd get much benefit from additionally keying that cache globally under the construction parameters rather than simply on the object itself and requiring that the user will keep a reference to the object for as long as he/she wants access to that cache.

[mo271]

comment:6

Thank you guys for replying to the ticket!

Ok, should the issue with UniqueRepresentation be moved to a new ticket?
And should we also open a ticket adressing the issue with the derivation of the base ring?

In this ticket simply wanted to make sure that is doesn't say "points in QQ", when the points are not rational.

[mkoeppe]

comment:7

One detail:

-            A point configuration in QQ^0 consisting of 0 points. The
+            A point configuration in affine 0-space over None 

Is this output really better?

[tscrim]

comment:8

Sorry for the delayed response, I've been traveling and on holidays.

I think you're right Nils, we probably should do away with the UniqueRepresentation behavior because it stores a lot of inputs, I don't think we do anything with coercion, and the user is not likely to create a lot of such inputs. I don't think we should use EqualityById as there are good ways to compare two such configurations. Anyways, yes, this should all be on another ticket.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

fbd2f0e	Merge branch 'develop' into 22132
a11676b	special case for the empty configuration

[sagetrac-git]

Changed commit from b5ac409 to a11676b

[mo271]

comment:10

thanks Matthias Köppe; the new representation was indeed not an improvement for the empty configuration. I added a special case to the routine.

[mkoeppe]

comment:11

Looking good.
Please do add tickets for the other issues discussed.

[mkoeppe]

Reviewer: Matthias Koeppe

[mkoeppe]

Author: Moritz Firsching

[vbraun]

Changed branch from u/moritz/22132 to a11676b