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:  sage6.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)
Change History (21)
comment:1 Changed 6 years ago by
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to enhancement
comment:2 followups: ↓ 3 ↓ 4 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:3 in reply to: ↑ 2 Changed 6 years ago by
Hi David!
Replying to roed:
 Rather than
@abstract_method
you should use therequired_methods
function fromsage.categories.Category
. Most of the functions on these categories should not actually be implemented on the category but rather included in the output ofrequired_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
 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 therequired_methods
function fromsage.categories.Category
. Most of the functions on these categories should not actually be implemented on the category but rather included in the output ofrequired_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 padic 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
comment:5 Changed 6 years ago by
 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
 Keywords sd52 added
comment:7 Changed 6 years ago by
 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
 Dependencies set to #14482
 Milestone changed from sage5.12 to sage6.0
comment:9 Changed 6 years ago by
 Cc defeo added
comment:10 Changed 6 years ago by
 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
 Dependencies #14482 deleted
comment:12 Changed 6 years ago by
 Dependencies set to #14482
I made some changes: if you're happy with them then feel free to mark as positive review.
comment:13 followup: ↓ 15 Changed 6 years ago by
 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
 Reviewers set to David Roe
comment:15 in reply to: ↑ 13 Changed 6 years ago by
Replying to caruso:
I don't just really understand why this ticket depends on #14482...
Because it uses sagegit 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
 Milestone changed from sage6.0 to sage6.1
comment:17 Changed 6 years ago by
 Commit set to 9e6ddb8711a56c0e0c68a1f06406d93ab8a8133f
comment:18 Changed 6 years ago by
 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:
b368c80  Merge branch 'develop' into ticket/14823

comment:19 Changed 6 years ago by
 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
 Resolution set to fixed
 Status changed from positive_review to closed
I'm glad you're working on this stuff. Here are some comments:
@abstract_method
you should use therequired_methods
function fromsage.categories.Category
. Most of the functions on these categories should not actually be implemented on the category but rather included in the output ofrequired_methods
.