Opened 2 years ago

Closed 2 years ago

#22132 closed defect (fixed)

Make base ring appear in self representation of PointConfiguration

Reported by: moritz Owned by:
Priority: trivial Milestone: sage-7.6
Component: geometry Keywords: base_ring
Cc: Merged in:
Authors: Moritz Firsching Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: a11676b (Commits) Commit: a11676b3870ff73d345c2a7a75be9b4dda66aa8e
Dependencies: Stopgaps:

Description

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. '

Change History (12)

comment:1 Changed 2 years ago by nbruin

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.

comment:2 Changed 2 years ago by moritz

  • Branch set to u/moritz/22132

comment:3 Changed 2 years ago by moritz

  • Commit set to b5ac4094a544522c865de3bef6301e5369553cd9
  • Status changed from new to needs_review

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:

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

comment:4 follow-up: Changed 2 years ago by 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>)
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.

comment:5 in reply to: ↑ 4 Changed 2 years ago by nbruin

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.

comment:6 Changed 2 years ago by moritz

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.

comment:7 Changed 2 years ago by mkoeppe

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?

comment:8 Changed 2 years ago by tscrim

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.

comment:9 Changed 2 years ago by git

  • Commit changed from b5ac4094a544522c865de3bef6301e5369553cd9 to a11676b3870ff73d345c2a7a75be9b4dda66aa8e

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

fbd2f0eMerge branch 'develop' into 22132
a11676bspecial case for the empty configuration

comment:10 Changed 2 years ago by moritz

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

comment:11 Changed 2 years ago by mkoeppe

  • Authors set to Moritz Firsching
  • Milestone changed from sage-7.5 to sage-7.6
  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

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

comment:12 Changed 2 years ago by vbraun

  • Branch changed from u/moritz/22132 to a11676b3870ff73d345c2a7a75be9b4dda66aa8e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.