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: sage-6.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:

Status badges

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 saraedum

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

  • Status changed from new to needs_review

comment:3 Changed 8 years ago by saraedum

  • Authors set to Julian Rueth
  • Milestone changed from sage-5.12 to sage-6.0
  • Modified changed from 09/13/13 13:44:47 to 09/13/13 13:44:47

comment:4 follow-up: Changed 8 years ago by pbruin

I think there is a more fundamental problem: in your example,

sage: isinstance(a, FieldElement)
False

The class FieldElement does have an is_unit() method that does the right thing, but evidently it is not used because of this.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by pbruin

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 ; follow-up: Changed 8 years ago by saraedum

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 pbruin

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 define is_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 non-zero. 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 for Fields()).

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 of Rings() 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 sage-devel discussion...

comment:8 follow-ups: Changed 8 years ago by 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 sage-devel if you don't mind. Can I just post what you wrote there? Or maybe it is less confusing if you do?

Last edited 8 years ago by saraedum (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 8 years ago by saraedum

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 sage-devel 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 ; follow-up: Changed 8 years ago by 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 sage-devel 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 sage-devel, but won't have a lot of time to write more.

comment:11 in reply to: ↑ 10 Changed 8 years ago by saraedum

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 sage-devel 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 sage-devel, 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 saraedum

  • Status changed from needs_review to needs_info
  • Work issues set to waiting for input from sage-devel

comment:13 Changed 8 years ago by saraedum

  • Status changed from needs_info to needs_review
  • Work issues waiting for input from sage-devel deleted

For the record: the discussion is here http://osdir.com/ml/sage-devel/2013-09/msg00086.html

comment:14 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:15 Changed 7 years ago by roed

  • Commit set to 953cf953cf7fa5a75cea895a7607c41d3a939be7
  • Status changed from needs_review to positive_review

Looks good.


New commits:

953cf95Remove is_unit() from sage.structure.element.Ring and move it to the category of Rings

comment:16 Changed 7 years ago by vbraun

  • Reviewers set to David Roe

Fill in your names, kids! ;-)

comment:17 Changed 7 years ago by vbraun

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