Opened 6 years ago

Closed 6 years ago

#14823 closed enhancement (fixed)

Categories of (C)DVR and (C)DVF

Reported by: caruso Owned by: nthiery
Priority: major Milestone: sage-6.1
Component: categories Keywords: categories, DVR, sd52
Cc: defeo Merged in:
Authors: Xavier Caruso Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: u/roed/ticket/14823 (Commits) Commit: b368c80b1b6c194a16b832ae28ed231eb1eb2cc2
Dependencies: #14482 Stopgaps:

Description

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

Attachments (1)

trac_14823_category_cdvr_cdvf.patch (22.5 KB) - added by caruso 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by caruso

  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

comment:2 follow-ups: Changed 6 years ago by roed

  • Status changed from needs_review to needs_work

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.

comment:3 in reply to: ↑ 2 Changed 6 years ago by nthiery

Hi David!

Replying to roed:

  • 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

comment:4 in reply to: ↑ 2 Changed 6 years ago by caruso

  • Status changed from needs_work to needs_info

Replying to roed:

  • 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?

Changed 6 years ago by caruso

comment:5 Changed 6 years ago by caruso

  • Status changed from needs_info to needs_review

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

comment:6 Changed 6 years ago by saraedum

  • Keywords sd52 added

comment:7 Changed 6 years ago by defeo

  • Branch set to public/ticket/14823
  • Created changed from 06/26/13 08:51:53 to 06/26/13 08:51:53
  • Modified changed from 09/02/13 12:57:38 to 09/02/13 12:57:38

comment:8 Changed 6 years ago by defeo

  • Dependencies set to #14482
  • Milestone changed from sage-5.12 to sage-6.0

comment:9 Changed 6 years ago by defeo

  • Cc defeo added

comment:10 Changed 6 years ago by roed

  • Branch changed from public/ticket/14823 to u/roed/ticket/14823
  • Modified changed from 09/04/13 11:37:00 to 09/04/13 11:37:00

comment:11 Changed 6 years ago by roed

  • Dependencies #14482 deleted

comment:12 Changed 6 years ago by roed

  • Dependencies set to #14482

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

comment:13 follow-up: Changed 6 years ago by caruso

  • Status changed from needs_review to positive_review

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

comment:14 Changed 6 years ago by caruso

  • Reviewers set to David Roe

comment:15 in reply to: ↑ 13 Changed 6 years ago by defeo

Replying to caruso:

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?

comment:16 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:17 Changed 6 years ago by vbraun

  • Commit set to 9e6ddb8711a56c0e0c68a1f06406d93ab8a8133f

Merge conflict, please fix.


New commits:

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

comment:18 Changed 6 years ago by git

  • Commit changed from 9e6ddb8711a56c0e0c68a1f06406d93ab8a8133f to b368c80b1b6c194a16b832ae28ed231eb1eb2cc2
  • Status changed from positive_review to needs_review

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

b368c80Merge branch 'develop' into ticket/14823

comment:19 Changed 6 years ago by roed

  • Status changed from needs_review to positive_review

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

comment:20 Changed 6 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.