Opened 20 months ago
Closed 14 months ago
#26498 closed enhancement (fixed)
each asymptotic ring gets its own term monoid factory
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  asymptotic expansions  Keywords:  
Cc:  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  4b1aab7 (Commits)  Commit:  4b1aab74e47fdc801396c5bcda7a32ffc4e37d43 
Dependencies:  Stopgaps: 
Description
Change History (11)
comment:1 Changed 20 months ago by
 Branch set to u/dkrenn/asyowntermmonoidfactory
comment:2 Changed 20 months ago by
 Commit set to 2e9317e8a9866babebf62bdd6f6e7763e440f474
comment:3 Changed 20 months ago by
 Status changed from new to needs_review
comment:4 Changed 20 months ago by
 Commit changed from 2e9317e8a9866babebf62bdd6f6e7763e440f474 to 40708fd108c586ee8443ff3aaed16c017a2887de
Branch pushed to git repo; I updated commit sha1. New commits:
40708fd  Trac #26498: fixup __init__ (somehow a line was not commited)

comment:5 Changed 19 months ago by
 Commit changed from 40708fd108c586ee8443ff3aaed16c017a2887de to 432dca4755323f302e0feb6738f46b1c3a500476
Branch pushed to git repo; I updated commit sha1. New commits:
432dca4  Trac #26498: method term_monoid

comment:6 Changed 14 months ago by
 Commit changed from 432dca4755323f302e0feb6738f46b1c3a500476 to 4b1aab74e47fdc801396c5bcda7a32ffc4e37d43
Branch pushed to git repo; I updated commit sha1. New commits:
4b1aab7  remove forgotten "TODO"

comment:7 Changed 14 months ago by
 Milestone changed from sage8.4 to sage8.8
 Reviewers set to Benjamin Hackl
 Status changed from needs_review to positive_review
I've looked through these changes and had one minor remark (the overlooked TODO in a docstring), this was already fixed via https://git.sagemath.org/sage.git/commit/?id=4b1aab74e47fdc801396c5bcda7a32ffc4e37d43.
Thus, I am happy. The patchbot tests seem to be falsenegatives, testing the code locally (after merging 8.7
) made all tests pass. Good to go.
comment:8 followup: ↓ 9 Changed 14 months ago by
Just a comment:
class TermMonoidFactory(UniqueRepresentation, UniqueFactory):
has a very strong code smell to me and makes me concerned about memory leaks. Maybe it is all okay, but it is a bit of a departure from normal use.
comment:9 in reply to: ↑ 8 Changed 14 months ago by
Replying to tscrim:
Just a comment:
class TermMonoidFactory(UniqueRepresentation, UniqueFactory):has a very strong code smell to me and makes me concerned about memory leaks. Maybe it is all okay, but it is a bit of a departure from normal use.
We have/need a parametrized factory; the desired behavior should be that of both base classes. Where/how do you think the memory leak appears?
comment:10 Changed 14 months ago by
What I am worried about is TermMonoidFactory
having a key G
to its construction, when then creates an object TM
(which has a weak ref from the factory) that then holds a strong ref to G
, creating a reference cycle with a strong reference with the UniqueRepresentation
key nailing it all in memory.
IMO, it also feels like the TermMonoidFactory
is better suited to being a parent that is its just a UniqueRepresentation
(not to mention all of the ugly doctests). It just is looking quite awkward to me.
However, this is not code I have an investment in, so the design decisions are your. So if there is no memory leak, then feel free to disregard my comment(s).
comment:11 Changed 14 months ago by
 Branch changed from u/dkrenn/asyowntermmonoidfactory to 4b1aab74e47fdc801396c5bcda7a32ffc4e37d43
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Trac #26498: convenience method term_monoid
Trac #26498: take care of term_monoid_factory in Functors