Opened 6 years ago

Closed 6 years ago

#15588 closed enhancement (fixed)

Cleanup of integer_mod_ring.py

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-6.1
Component: doctest coverage Keywords:
Cc: ncohen Merged in:
Authors: Travis Scrimshaw Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: public/rings/cleanup_integer_mod_ring (Commits) Commit: 0e1a46b94e5094921fe35561e75c9369d7cd7d92
Dependencies: #15369 Stopgaps:

Description

  • Bring coverage to 100%; a missing doctest for extension().
  • Use cached_method instead of a custom cache.
  • Fix docstring formatting.
  • Make it python3 compliant (at least as far as I know).

All of these are good things to do.

Change History (7)

comment:1 Changed 6 years ago by tscrim

  • Status changed from new to needs_review

comment:2 follow-up: Changed 6 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Ahahaahah. Funny. I barely know what all these functions do, though I was always able to check that the code was equivalent to what it was previously. Sooooooo it's all good.

Even though we don't agree on style issues. It looks like you prefer to have many "if/return, if/return,if/return" while I prefer "elif/return", and for some reason you don't like "else" at all.

Ahahaah. Anyway, it's good to go, thank you for that !

And I have to read this awful thing soon, for I can't go on having to complain each time the categories are involved. http://www.sagemath.org/doc/reference/categories/sage/categories/primer.html

I feel like I should do more. I should study it very intently, in order to be able to explain with the right vocabulary WHY everything related to categories/parents/elements should be removed from Sage :-P

Besides, this example for your docstring is reaaaaaaaallllyyy scary :

        sage: F19 = IntegerModRing(19, category = Fields())                                                                                                                                    
        sage: F19.category().is_subcategory(Fields())                                                                                                                                          
        True                                                                                                                                                                                   
	sage: F23 = IntegerModRing(23)                                                                                                                                                         
        sage: F23.category().is_subcategory(Fields())                                                                                                                                          
	False                                                                                                                                                                                  
        sage: F23 in Fields()                                                                                                                                                                  
        True                                                                                                                                                                                   
        sage: F23.category().is_subcategory(Fields())                                                                                                                                          
        True   

Anyway. I tried to pay very close attention, and all I could spot in your commit is that I would have written `GF(p) -> Z/pZ` instead of ``GF(p) -> Z/pZ``. Which I guess is only style again :-P

Whatever !

Nathann

comment:3 in reply to: ↑ 2 ; follow-up: Changed 6 years ago by tscrim

Replying to ncohen:

Even though we don't agree on style issues. It looks like you prefer to have many "if/return, if/return,if/return" while I prefer "elif/return", and for some reason you don't like "else" at all.

I just get annoyed when there's something like:

if cond:
    special/small case
    return foo
else:
    a long code block
    return foobar

and I just try to be consistent with that overall. Plus the extra indents can play hell on my sanity when going from 5 indents to 1 (or was it 6 or 4... :P). Okay, I'll stop ranting.

Ahahaah. Anyway, it's good to go, thank you for that !

Thank you for doing the review!

And I have to read this awful thing soon, for I can't go on having to complain each time the categories are involved. http://www.sagemath.org/doc/reference/categories/sage/categories/primer.html

I think Simon's "implementing a new parent" (or something like that) tutorial is a better starting point.

Besides, this example for your docstring is reaaaaaaaallllyyy scary :

It has to do with refining the category and doing primality checks that one may not want todo when constructing the object. There's a topic on sage-devel and a ticket on this as I recall. For example, do you know if ZZ / 10000000000000069 ZZ is a field? It is in fact, and now that you do know it, you can do things like you would with other fields. That's basically what that docstring is about.

Anyway. I tried to pay very close attention, and all I could spot in your commit is that I would have written `GF(p) -> Z/pZ` instead of ``GF(p) -> Z/pZ``. Which I guess is only style again :-P

I just made it verbatim, but since it's not seen in the built doc (in fact, almost noone will ever see that), I didn't put much thought into it. Feel free to change it.

Thanks again Nathann,
Travis

comment:4 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by jdemeyer

Replying to tscrim:

I just get annoyed when there's something like:

if cond:
    special/small case
    return foo
else:
    a long code block
    return foobar

You may be annoyed by that style, but I am annoyed by people rewriting code for no other reason than it suits their own personal style. When it's newly added code, it makes sense to have a good consistent style. But I don't think you should rewrite existing code into a particular style. But since I'm not the reviewer of this ticket, you're free to ignore this opinion.

comment:5 Changed 6 years ago by ncohen

I am the reviewer of this ticket, and I try to make efforts to not create arguments with everybody around me every time we have to agree on something. I find it quite hard. I agree with Jeroen. But it's already hell to get people to acknowledge the existence of bugs and fix them, soooooo well... :-P

Nathann

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

Replying to jdemeyer:

You may be annoyed by that style, but I am annoyed by people rewriting code for no other reason than it suits their own personal style. When it's newly added code, it makes sense to have a good consistent style. But I don't think you should rewrite existing code into a particular style. But since I'm not the reviewer of this ticket, you're free to ignore this opinion.

I value your opinion. I believe that the format I use is more readable and can sometimes simplify the logic, but perhaps it's because I'm just used to it. I'll refrain from making such changes in the future.

comment:7 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.