Opened 11 years ago

Closed 11 years ago

#8562 closed enhancement (fixed)

Categories for IntegerMod rings

Reported by: nthiery Owned by: AlexGhitza
Priority: major Milestone: sage-4.5.2
Component: algebra Keywords: categories, integer mod rings
Cc: sage-combinat Merged in: sage-4.5.2.alpha0
Authors: Nicolas M. Thiéry Reviewers: John Palmieri, Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by nthiery)

After this patch, IntegerModRing?'s inherit properly from categories:

    sage: Z3 = IntegerModRing(3)
    sage: Z3.category()
    Category of commutative rings
    sage: TestSuite(Z3).run(verbose = True)
    running ._test_additive_associativity() . . . pass
    running ._test_an_element() . . . pass
    running ._test_associativity() . . . pass
    running ._test_category() . . . pass
    running ._test_elements() . . . 
      Running the test suite of self.an_element()
      running ._test_category() . . . pass
      running ._test_not_implemented_methods() . . . pass
      running ._test_pickling() . . . pass
      pass
    running ._test_not_implemented_methods() . . . pass
    running ._test_one() . . . pass
    running ._test_pickling() . . . pass
    running ._test_prod() . . . pass
    running ._test_some_elements() . . . pass
    running ._test_zero() . . . pass

And this makes the cool features from #7555 work for Z/nZ.

Potential conflict with #8218 (which has higher priority)

For a later ticket, see: running design discussion on:

http://groups.google.com/group/sage-devel/t/21e21e1ec9cd21fe

Attachments (2)

trac_8562-category-integer_mod_ring-nt.patch (10.7 KB) - added by nthiery 11 years ago.
trac_8562-rebased.patch (10.9 KB) - added by jhpalmieri 11 years ago.
rebased version. apply only this patch.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by nthiery

  • Status changed from new to needs_work
  • Work issues set to (didn't run full tests yet)

comment:2 Changed 11 years ago by nthiery

  • Status changed from needs_work to needs_review

Ok, the tests passed fine up to some trivialities (*_with_category class name changes). I'll fix this and upload an updated patch soon. The review can start in parallel

comment:3 Changed 11 years ago by rbeezer

This applies, builds and limited testing (prime and composite orders) indicates that it plays nicely with "addition tables" at #7555 which rely heavily on the category framework. Didn't run tests, but witnessed no problems otherwise.

Good to see how easy it is to insert a new object into the category framework.

I'll come back when the patch is completed.

comment:4 follow-up: Changed 11 years ago by davidloeffler

Just a heads-up: this looks rather like it might clash with #8218, which is the first of several patches by David Roe which do a substantial amount of work improving finite fields. #8218 has been held up for ages because it moves loads of files around (without substantially changing their content) so even small changes to finite fields will cause conflicts, and there is a lot of really good code waiting on it, so it would be a shame to have to put it off even longer.

David

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

Replying to davidloeffler:

Just a heads-up: this looks rather like it might clash with #8218, which is the first of several patches by David Roe which do a substantial amount of work improving finite fields. #8218 has been held up for ages because it moves loads of files around (without substantially changing their content) so even small changes to finite fields will cause conflicts, and there is a lot of really good code waiting on it, so it would be a shame to have to put it off even longer.

Thanks for the notice. There is no urgency for that one, so sure, if there is any conflict, #8218 should go first.

David: I won't be touching this patch further. Feel free to update / refactor / merge /... it within the other series of patch whenever it feels right.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 11 years ago by nthiery

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Work issues changed from (didn't run full tests yet) to Designe decision for IntegerModRing(5).category()

David: I won't be touching this patch further. Feel free to update / refactor / merge /... it within the other series of patch whenever it feels right.

Note: I meant David Roe, but any other David is welcome too :-)

Oh: would you agree to take over that patch, and finalize it (or ping me) when it's ripe to get in?

(then I could forget about it).

comment:7 Changed 11 years ago by nthiery

  • Description modified (diff)
  • Reviewers Robert Beezer deleted
  • Status changed from needs_work to needs_review
  • Type changed from defect to enhancement
  • Work issues Designe decision for IntegerModRing(5).category() deleted

comment:8 in reply to: ↑ 6 Changed 11 years ago by nthiery

Replying to nthiery:

David: I won't be touching this patch further. Feel free to update / refactor / merge /... it within the other series of patch whenever it feels right.

Well, actually I did. But I should be done now, unless I notice a test failure.

comment:9 Changed 11 years ago by jhpalmieri

Here's a rebased patch. It looks good and all tests pass for me so I'm willing to give it a positive review, but since I made the rebased patch, can someone else (Nicolas, for example) take a look at it also?

comment:10 follow-up: Changed 11 years ago by nthiery

Hi John!

Thanks much for rebasing the patch. I looked through the changes, and am happy to give my green light, up to three minor comments:

  • Is the convention to use as ticket summary "trac 8562:" or "#8562:"? (I personally prefer the later).
  • With the updated patch, sage -coverage complains because of the absence of #indirect doctest for create_object in sage/rings/finite_rings/integer_mod_ring.py. Just wanted to check; if this is voluntary, because you consider that this requires better tests, that's all fine with me.
  • I like the options nodates=1 and showfunc = 1 of hg :-)

I let you set up the positive review as you feel appropriate.

Thanks again,

Nicolas

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

  • Reviewers set to John Palmieri, Rob Beezer

Replying to nthiery:

Is the convention to use as ticket summary "trac 8562:" or "#8562:"? (I personally prefer the later).

I think either is fine. I've been using "trac 8562" for a while and have not had any complaints from release managers.

  • With the updated patch, sage -coverage complains because of the absence of #indirect doctest for create_object in sage/rings/finite_rings/integer_mod_ring.py. Just wanted to check; if this is voluntary, because you consider that this requires better tests, that's all fine with me.

This wasn't voluntary, it was an oversight. I'll fix it.

  • I like the options nodates=1 and showfunc = 1 of hg :-)

Nice, I didn't know about those.

Changed 11 years ago by jhpalmieri

rebased version. apply only this patch.

comment:12 Changed 11 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:13 Changed 11 years ago by mpatel

  • Merged in set to sage-4.5.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.