Categories of (C)DVR and (C)DVF

[xcaruso]

Here is a small patch defining categories of (complete) discrete valuation rings and (complete) discrete valuation fields.

Depends on #14482

CC: @defeo

Component: categories

Keywords: DVR, sd52

Author: Xavier Caruso

Branch/Commit: u/roed/ticket/14823 @ b368c80

Reviewer: David Roe

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

[roed314]

comment:2

I'm glad you're working on this stuff. Here are some comments:

• There are test failures to fix but they look simple.
• Rather than @abstract_method you should use the required_methods function from sage.categories.Category. Most of the functions on these categories should not actually be implemented on the category but rather included in the output of required_methods.
• In power series rings it's probably worth giving some examples with infinite precision to highlight the difference with p-adic rings.

[nthiery]

comment:3

Hi David!

Replying to @roed314:

• Rather than @abstract_method you should use the
  required_methods function from sage.categories.Category. Most
  of the functions on these categories should not actually be
  implemented on the category but rather included in the output of
  required_methods.

I am not sure I understand your point. Looking at the patch, those
methods are not implemented on the category (just declared as abstract
methods), and it's those declarations that get them included in
required_methods.

Other than this, just for checking (and showing off my incompetence on
the topic at hand), it's on purpose that a discrete valuation field is
not a discrete valuation ring?

Cheers,
Nicolas

[xcaruso]

comment:4

Replying to @roed314:

• There are test failures to fix but they look simple.

Fixed.

• Rather than @abstract_method you should use the required_methods function from sage.categories.Category. Most of the functions on these categories should not actually be implemented on the category but rather included in the output of required_methods.

cf Nicolas' answer: I think that the "right way" is to decorate these functions by @abstract_method, so that they are automatically discovered by required_methods.

• In power series rings it's probably worth giving some examples with infinite precision to highlight the difference with p-adic rings.

I don't understand what you are saying: do you mean that I should add some doctests in sage.rings.power_series_ring and sage.rings.power_series_ring_element to highlight this difference?

[xcaruso]

comment:5

Attachment: trac_14823_category_cdvr_cdvf.patch.gz

I realized that my patch was not compatible with #14084. I just posted a revised version of my patch.

[saraedum]

Changed keywords from DVR to DVR, sd52

[defeo]

Branch: public/ticket/14823

[defeo]

Dependencies: #14482

[roed314]

Changed branch from public/ticket/14823 to u/roed/ticket/14823

[roed314]

Changed dependencies from #14482 to none

[roed314]

comment:12

I made some changes: if you're happy with them then feel free to mark as positive review.

[roed314]

Dependencies: #14482

[xcaruso]

comment:13

I'm happy with your modifications. I don't just really understand why this ticket depends on #14482...

[xcaruso]

Reviewer: David Roe

[defeo]

comment:15

Replying to @xcaruso:

I don't just really understand why this ticket depends on #14482...

Because it uses sage-git with the dev scripts. It cannot really go in before #14482 gets in (and it might need fiddling with the branch, then, I'll keep an eye on it).

Any idea why David's branch does not have a link?

[vbraun]

Commit: 9e6ddb8

[vbraun]

comment:17

Merge conflict, please fix.

---

New commits:

664322f	Trac #14823: Category of (complete) discrete valuation rings/fields
9e6ddb8	Reviewer changes: delete is_cdvr functions and friends, add abstract_method to precision_absolute and precision_relative, add some tests

[sagetrac-git]

Changed commit from 9e6ddb8 to b368c80

[sagetrac-git]

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

b368c80

Merge branch 'develop' into ticket/14823

[roed314]

comment:19

I've trivially fixed the merge conflict, so I'm marking it back to positive review.