Opened 8 years ago
Closed 7 years ago
#15192 closed enhancement (fixed)
add is_unit() for residue fields
Reported by:  saraedum  Owned by:  

Priority:  trivial  Milestone:  sage6.1 
Component:  categories  Keywords:  
Cc:  Merged in:  
Authors:  Julian Rueth  Reviewers:  David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  u/saraedum/ticket/15192 (Commits, GitHub, GitLab)  Commit:  953cf953cf7fa5a75cea895a7607c41d3a939be7 
Dependencies:  Stopgaps: 
Description
Currently, residue fields have no is_unit
:
sage: K.<z> = CyclotomicField(7) sage: P = K.factor(17)[0][0] sage: ff = K.residue_field(P) sage: a = ff(z) sage: a.is_unit() NotImplementedError
They are fields but the implementation from Fields.ElementMethods
is not used because the generic ring class defines a trivial is_unit()
already. This ticket moves that method to the category of rings which gives a working is_unit()
for residue fields.
Change History (17)
comment:1 Changed 8 years ago by
 Branch set to u/saraedum/ticket/15192
 Created changed from 09/13/13 13:33:04 to 09/13/13 13:33:04
 Modified changed from 09/13/13 13:33:04 to 09/13/13 13:33:04
comment:2 Changed 8 years ago by
 Status changed from new to needs_review
comment:3 Changed 8 years ago by
 Milestone changed from sage5.12 to sage6.0
 Modified changed from 09/13/13 13:44:47 to 09/13/13 13:44:47
comment:4 followup: ↓ 5 Changed 8 years ago by
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 8 years ago by
Actually, it seems that every finite field element 'a' has the above problem. Finite field elements should be made to inherit from field elements!
comment:6 in reply to: ↑ 5 ; followup: ↓ 7 Changed 8 years ago by
Thanks for having a look at this.
Replying to pbruin:
Actually, it seems that every finite field element 'a' has the above problem.
That's scary.
Finite field elements should be made to inherit from field elements!
I agree. I wonder if there is a reason why they don't?
In any case, should Ring
really define is_unit
? Isn't the idea that elements figure these things out depending on their parent, i.e., whether they are a unit depends on the parent. Putting this into the category framework seems to be the right thing. (Notice that is_unit
was already there for Fields()
). So is there any benefit of such an implementation in Ring
? Don't all rings live in the category of Rings()
now?
comment:7 in reply to: ↑ 6 Changed 8 years ago by
Replying to saraedum:
Finite field elements should be made to inherit from field elements!
I agree. I wonder if there is a reason why they don't?
Probably because they also derive from IntegerMod
(for prime fields) and FinitePolyExtElement
, respectively; I assume one could use multiple inheritance, but it might be tricky. However, this should certainly be fixed.
In any case, should
Ring
really defineis_unit
?
(I assume you mean RingElement
...)
Isn't the idea that elements figure these things out depending on their parent, i.e., whether they are a unit depends on the parent.
That is clear; talking about elements doesn't even make much sense without mentioning parent sets. But there isn't really any 'figuring out' to be done here: if the element knows that it is a field element, then it knows that being a unit is equivalent to being nonzero. An element can either know that is is a field element because it inherits from FieldElement
, or otherwise (e.g. if the parent can only determine at runtime if it is a field) using the is_field()
method of the parent.
Putting this into the category framework seems to be the right thing.
I don't agree, for the same reason why I wrote the comment at http://trac.sagemath.org/ticket/13441#comment:20. Parents describe relations between elements; e.g. a ring is a set whose elements satisfy certain axioms. Categories are on a higher level; they describe relations between objects, and it shouldn't matter too much what the elements of these objects are, or even if the objects are sets at all.
Whether an element is unit depends on relations satisfied within its parent, not on relations between the parent and other parents. In other words, the category its parent is in should not play any role. It is true that some categories (like the category of rings) are in a sense defined by relations between the elements of objects, but exploiting this to influence the behaviour of elements is confusing two different levels of abstraction, in my opinion.
Hence, I don't see a good mathematical reason for putting methods in Ring.ElementMethods
instead of RingElement
. I can see why one would want a Category
to have ObjectMethods
(which are now called ParentMethods
) and perhaps MorphismMethods
(which only exist in a doctest). With ElementMethods
, however, I somehow get the feeling that the authors of the category framework are aiming for world domination!
(Notice that
is_unit
was already there forFields()
).
Yes, but FieldElement
still has its own is_unit()
. I would expect that RingElement
should have one, too, and that these are the correct locations for these methods, rather than *.ElementMethods
.
So is there any benefit of such an implementation in
Ring
? Don't all rings live in the category ofRings()
now?
Maybe, but I don't think this is a good argument for using the category framework here. I think the benefit of putting ring element methods in RingElement
is that it simply seems the most logical place.
Sorry for the long sermon; this might have been more appropriate for a sagedevel discussion...
comment:8 followups: ↓ 9 ↓ 10 Changed 8 years ago by
Does trac send out another email when I edit this? Anyway, I'm confused. I'm actually not sure what ElementMethods
are good for so I'd rather continue this discussion on sagedevel if you don't mind. Can I just post what you wrote there? Or maybe it is less confusing if you do?
comment:9 in reply to: ↑ 8 Changed 8 years ago by
Replying to saraedum:
Does trac send out another email when I edit this? Anyway, I'm confused. I'm actually not sure what
ElementMethods
are good for so I'd rather continue this discussion on sagedevel if you don't mind. Can I just post what you wrote there? Or maybe it is less confusing if you do?
I edited my comment and trac does not send out an email, so I'm not sure if you noticed. Could you comment on this please? Thanks.
comment:10 in reply to: ↑ 8 ; followup: ↓ 11 Changed 8 years ago by
Replying to saraedum:
Does trac send out another email when I edit this?
No, it doesn't. :)
Anyway, I'm confused. I'm actually not sure what
ElementMethods
are good for so I'd rather continue this discussion on sagedevel if you don't mind. Can I just post what you wrote there? Or maybe it is less confusing if you do?
Sure, I'd be happy if more people gave their opinion on this, and if someone who knows more about the category framework (maybe one of the authors) could explain the reason for ElementMethods
.
I can post a message to sagedevel, but won't have a lot of time to write more.
comment:11 in reply to: ↑ 10 Changed 8 years ago by
Replying to pbruin:
Replying to saraedum:
Does trac send out another email when I edit this?
No, it doesn't. :)
Anyway, I'm confused. I'm actually not sure what
ElementMethods
are good for so I'd rather continue this discussion on sagedevel if you don't mind. Can I just post what you wrote there? Or maybe it is less confusing if you do?Sure, I'd be happy if more people gave their opinion on this, and if someone who knows more about the category framework (maybe one of the authors) could explain the reason for
ElementMethods
.I can post a message to sagedevel, but won't have a lot of time to write more.
Thanks. And no worries, it's great that you had a look at this in the first place.
comment:12 Changed 8 years ago by
 Status changed from needs_review to needs_info
 Work issues set to waiting for input from sagedevel
comment:13 Changed 8 years ago by
 Status changed from needs_info to needs_review
 Work issues waiting for input from sagedevel deleted
For the record: the discussion is here http://osdir.com/ml/sagedevel/201309/msg00086.html
comment:14 Changed 7 years ago by
 Milestone changed from sage6.0 to sage6.1
comment:15 Changed 7 years ago by
 Commit set to 953cf953cf7fa5a75cea895a7607c41d3a939be7
 Status changed from needs_review to positive_review
Looks good.
New commits:
953cf95  Remove is_unit() from sage.structure.element.Ring and move it to the category of Rings

comment:17 Changed 7 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
I think there is a more fundamental problem: in your example,
The class
FieldElement
does have anis_unit()
method that does the right thing, but evidently it is not used because of this.