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

change for p-adic rings #20310

Closed
xcaruso opened this issue Mar 27, 2016 · 85 comments
Closed

change for p-adic rings #20310

xcaruso opened this issue Mar 27, 2016 · 85 comments

Comments

@xcaruso
Copy link
Contributor

xcaruso commented Mar 27, 2016

Started as a proposal to change precision of rings, this is now a more general change method.

Depends on #23331

CC: @saraedum @roed314

Component: padics

Keywords: precision

Author: David Roe

Branch/Commit: 86b58de

Reviewer: Julian Rüth, Xavier Caruso

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

@xcaruso xcaruso added this to the sage-7.2 milestone Mar 27, 2016
@xcaruso
Copy link
Contributor Author

xcaruso commented Mar 27, 2016

comment:1

As discussed in Sage Days 71, I tried to implement this method using the construction functor but it seems that the latter is only defined for Zp or Qp but not for extensions.
So, what should I do? Implement the methods construction as well? Something else?

@xcaruso
Copy link
Contributor Author

xcaruso commented Mar 27, 2016

Branch: u/caruso/change_precision

@roed314
Copy link
Contributor

roed314 commented Mar 27, 2016

Commit: 9ca8df8

@roed314
Copy link
Contributor

roed314 commented Mar 27, 2016

comment:3

Implementing construction is a good idea anyway, since they're used in coercion. And until we change extensions to be defined by exact polynomials, you'll need to raise the precision of the base ring as well, so it's probably necessary.


New commits:

9ca8df8Generic function change_precision

@roed314
Copy link
Contributor

roed314 commented Jun 14, 2017

Changed branch from u/caruso/change_precision to u/roed/change_precision

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

Changed commit from 9ca8df8 to 53d2446

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

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

e16f79fAdd construction method for extensions of p-adic rings and fields, switch implementation of fraction_field and integer_ring
a88b630Fixing problems revealed by tests
fdd03cbMerge branch 'u/roed/padic-floats' of git://trac.sagemath.org/sage into t/20310/change_precision
53d2446Added ability to edit show_prec to p-adic factory

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

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

55c335eFixing doctest problems in the new change method for p-adic rings/fields

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

Changed commit from 53d2446 to 55c335e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

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

fcc8aa5Fixing various problems with the new change method

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

Changed commit from 55c335e to fcc8aa5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

Changed commit from fcc8aa5 to 3b2d94a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

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

3b2d94aRemove old code for fraction_field and integer_ring from padic_extension_generic

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

Changed commit from 3b2d94a to f386dc6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

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

f386dc6Fix bug in ring_of_integers

@saraedum

This comment has been minimized.

@saraedum saraedum changed the title change_precision for p-adic rings change for p-adic rings Jun 16, 2017
@xcaruso
Copy link
Contributor Author

xcaruso commented Jun 16, 2017

comment:12

I'm a bit sceptical about the possibility of changing p (and even q). Can you show me a situation where this can be needed?

Adding show_prec is nice but is it really the subject of this ticket?

Doctests implying the keyword base are missing. The same is true for show_prec. More generally, I suggest to add many more doctests ensuring that your code works in a wide range of situations (for ramified, unramified extensions, for extensions constructing via CompletionFunction and AlgebraicExtensionFunctor, etc.)

@saraedum
Copy link
Member

comment:13

Xavier: Thanks for having a look at this as well. We discussed the changing of p and q briefly and actually I find it very useful. Often I am only really interested in the ramified part of a tower of extensions (on my fork that already does more general towers.) But for some computation to work out, I need a certain unramified element at the bottom, so I don't want to manually reconstruct the whole tower, so raising and lowering q is quite useful. Changing p is mostly interesting in general extensions where you want to see how a certain polynomial behaves wrt to different primes. For the Eisenstein extensions that we support atm it is not too useful.

I think it's Ok to put the show_prec in here, I'm fine with reviewing it at least :)

I agree that doctests for base and show_prec should be added. However, I think that in general not all possible cases should be covered by explicit doctests. It usually makes the documentation overwhelming. If we want to have coverage, we should write unit tests for that instead. That's btw one of the topics that we want to push in Burlington! Imho, the testing for change is Ok except for the untested keywords.

@saraedum
Copy link
Member

comment:14

David: should get_key_base set show_prec? Otherwise, show_prec=None and show_prec=True/False produces two non-identical parents for the same ring I think.

@saraedum
Copy link
Member

Author: David Roe

@saraedum
Copy link
Member

Reviewer: Julian Rüth

@saraedum
Copy link
Member

Work Issues: show_prec in factory keys

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2017

Changed commit from 3142701 to 138d939

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2017

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

138d939Fix string representation doctest from #22103

@roed314
Copy link
Contributor

roed314 commented Aug 3, 2017

Diff against 23331

@roed314
Copy link
Contributor

roed314 commented Aug 3, 2017

comment:56

Attachment: 20310_over_23331.diff.gz

For ease of reviewing, I've attached a diff against #23331.

@vbraun
Copy link
Member

vbraun commented Aug 5, 2017

comment:57

PDF docs don't build

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2017

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

b18929aMerge branch 'develop' into t/20310/change_precision

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2017

Changed commit from 138d939 to b18929a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 6, 2017

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

86b58deFix latex error

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 6, 2017

Changed commit from b18929a to 86b58de

@roed314
Copy link
Contributor

roed314 commented Aug 8, 2017

comment:60

Alright, make succeeds at building the docs now.

@saraedum
Copy link
Member

saraedum commented Aug 8, 2017

comment:61

daucher still reports a failing docbuild. I guess that's because I told it not to clean the docs when rebuilding.

@vbraun
Copy link
Member

vbraun commented Aug 11, 2017

Changed branch from u/roed/change_precision to 86b58de

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

4 participants