Opened 6 years ago
Closed 6 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, GitHub, GitLab) | 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 6 years ago by
comment:2 Changed 6 years ago by
- Branch set to u/moritz/22132
comment:3 Changed 6 years ago by
- 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:
b5ac409 | changing self-representation of PointConfiguration (and all the doctests)
|
comment:4 follow-up: ↓ 5 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
- Commit changed from b5ac4094a544522c865de3bef6301e5369553cd9 to a11676b3870ff73d345c2a7a75be9b4dda66aa8e
comment:10 Changed 6 years ago by
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 6 years ago by
- 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 6 years ago by
- Branch changed from u/moritz/22132 to a11676b3870ff73d345c2a7a75be9b4dda66aa8e
- Resolution set to fixed
- Status changed from positive_review to closed
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?
and the derivation of the base ring seems fishy too:
That seems wrong.
For the string representation, I'd go for
The reality is,
QQ,AA,RDF
aren't actually print names of rings. They are just convenience bindings to commonly occurring rings.