#26498 closed enhancement (fixed)

each asymptotic ring gets its own term monoid factory

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-8.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 dkrenn

  • Branch set to u/dkrenn/asy-own-term-monoid-factory

comment:2 Changed 20 months ago by git

  • Commit set to 2e9317e8a9866babebf62bdd6f6e7763e440f474

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

0e45ca8Trac #26498: convenience method term_monoid
2e9317eTrac #26498: take care of term_monoid_factory in Functors

comment:3 Changed 20 months ago by dkrenn

  • Authors set to Daniel Krenn
  • Status changed from new to needs_review

comment:4 Changed 20 months ago by git

  • Commit changed from 2e9317e8a9866babebf62bdd6f6e7763e440f474 to 40708fd108c586ee8443ff3aaed16c017a2887de

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

40708fdTrac #26498: fixup __init__ (somehow a line was not commited)

comment:5 Changed 19 months ago by git

  • Commit changed from 40708fd108c586ee8443ff3aaed16c017a2887de to 432dca4755323f302e0feb6738f46b1c3a500476

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

432dca4Trac #26498: method term_monoid

comment:6 Changed 14 months ago by git

  • Commit changed from 432dca4755323f302e0feb6738f46b1c3a500476 to 4b1aab74e47fdc801396c5bcda7a32ffc4e37d43

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

4b1aab7remove forgotten "TODO"

comment:7 Changed 14 months ago by behackl

  • Milestone changed from sage-8.4 to sage-8.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 false-negatives, testing the code locally (after merging 8.7) made all tests pass. Good to go.

comment:8 follow-up: Changed 14 months ago by 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.

comment:9 in reply to: ↑ 8 Changed 14 months ago by dkrenn

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 tscrim

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 vbraun

  • Branch changed from u/dkrenn/asy-own-term-monoid-factory to 4b1aab74e47fdc801396c5bcda7a32ffc4e37d43
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.