Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UniqueFactory for DirichletGroup #15898

Closed
saraedum opened this issue Mar 6, 2014 · 14 comments
Closed

UniqueFactory for DirichletGroup #15898

saraedum opened this issue Mar 6, 2014 · 14 comments

Comments

@saraedum
Copy link
Member

saraedum commented Mar 6, 2014

Dirichlet groups use a generic dictionary to cache objects (instead of UniqueRepresentation or UniqueFactory). This ticket replaces this generic cache with a UniqueFactory implementation.

Component: modular forms

Author: Julian Rueth

Branch/Commit: ce3dfc0

Reviewer: Peter Bruin

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

@saraedum saraedum added this to the sage-6.2 milestone Mar 6, 2014
@saraedum
Copy link
Member Author

saraedum commented Mar 6, 2014

Branch: u/saraedum/ticket/15898

@saraedum
Copy link
Member Author

saraedum commented Mar 6, 2014

Author: Julian Rueth

@saraedum
Copy link
Member Author

saraedum commented Mar 6, 2014

New commits:

55bf7beUse a UniqueFactory to cache instances of DirichletGroup

@saraedum
Copy link
Member Author

saraedum commented Mar 6, 2014

Commit: 55bf7be

@pjbruin
Copy link
Contributor

pjbruin commented Apr 8, 2014

comment:3

Looks good (although I haven't tested it yet). Two questions:

  • why include base_ring in key? It can be recovered as the parent of zeta, and isn't used in create_object().
  • could you add a doctest to show that it works as intended, i.e. really creates a unique instance?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 9, 2014

Changed commit from 55bf7be to 3d01887

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 9, 2014

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

ebe6a39Doctest to illustrate that caching of DirichletGroups works
0521e97Doctest to illustrate that base_ring must be a part of the cache key created by the DirichletGroup factory
3d01887cleaned up docstring of DirichletGroup

@pjbruin
Copy link
Contributor

pjbruin commented Apr 9, 2014

comment:6

Replying to @sagetrac-git:

0521e97 Doctest to illustrate that base_ring must be a part of the cache key created by the DirichletGroup factory
Nice, I hadn't thought of the fact that two roots of unity can still compare as equal when they are in different fields! Now testing.

@pjbruin
Copy link
Contributor

pjbruin commented Apr 9, 2014

Reviewer: Peter Bruin

@vbraun
Copy link
Member

vbraun commented Apr 10, 2014

comment:8

Conflicts with #15990, please fix

@pjbruin
Copy link
Contributor

pjbruin commented Apr 21, 2014

Changed branch from u/saraedum/ticket/15898 to u/pbruin/15898-DirichletGroup_unique

@pjbruin
Copy link
Contributor

pjbruin commented Apr 21, 2014

comment:9

Merged with 6.2.beta8.

@pjbruin
Copy link
Contributor

pjbruin commented Apr 21, 2014

Changed commit from 3d01887 to ce3dfc0

@vbraun
Copy link
Member

vbraun commented Apr 22, 2014

Changed branch from u/pbruin/15898-DirichletGroup_unique to ce3dfc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants