Opened 8 years ago

Closed 8 years ago

#12261 closed enhancement (fixed)

Bring Doctest coverage for element_ext_pari.py to 100%

Reported by: roed Owned by: mvngu
Priority: major Milestone: sage-5.0
Component: doctest coverage Keywords:
Cc: robertwb Merged in: sage-5.0.beta3
Authors: David Roe Reviewers: Karl-Dieter Crisman, Aly Deines
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Part of metaticket #12024

Attachments (4)

12261.patch (7.5 KB) - added by roed 8 years ago.
12261.2.patch (7.6 KB) - added by roed 8 years ago.
Apply this patch only
12261.3.patch (8.9 KB) - added by roed 8 years ago.
Most recent update
12261.4.patch (8.6 KB) - added by roed 8 years ago.
Even more recent update. It would be nice to be able to delete patches.…

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by roed

comment:1 Changed 8 years ago by roed

  • Authors set to David Roe
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman

I'm finding it hard to find any problems with this that aren't nearly trivial. Comments that may or may not warrant addressing:

  • Typo
    The has of this element 
    
  • def __compat(self, other): removed because...? (I assume this is obvious to someone familiar with this code, but they haven't reviewed this.)
  • I really like that you actually mention that the doctests are indirect when they are. We need to do that more.
  • You got 'em:
    $ ../../sage -coverage ../../devel/sage/sage/rings/finite_rings/element_ext_pari.py
    ----------------------------------------------------------------------
    ../../devel/sage/sage/rings/finite_rings/element_ext_pari.py
    SCORE ../../devel/sage/sage/rings/finite_rings/element_ext_pari.py: 100% (31 of 31)
    ----------------------------------------------------------------------
    
    and testing seems fine.
  • Although not really part of this ticket, one could add this to the reference manual; if so, might as well change See self.square_root() to See :meth:`square_root` or whatever the right syntax is.
  • Really picky; is it Pari or pari, officially? I feel like this was unified at some point in our docs.
  • There is a typo that has nothing to do with this ticket but should be fixed - probabalistic

Let me know what you want to deal with on a refresh (probably at least the typos) and otherwise this should go in.

Changed 8 years ago by roed

Apply this patch only

comment:3 Changed 8 years ago by roed

Sorry: I should have made a reviewer patch rather than a whole new one. But I'd already qrefreshed...

comment:4 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_work
  • Work issues set to commit message, PARI (maybe?)

That's ok. Patch is fine.

I don't feel comfortable giving final positive review because of the __compat issue - again, I'm sure this is obvious to those in the know, but I don't want to be responsible for removing a function it turns out is crucial to someone (even an underscore function).

Also (less crucially) now I realize that I think the folks in Bordeaux really want PARI and not Pari... I just don't know, and hate all that stuff, but it's probably important to them.

Jeroen probably also won't accept this due to the commit "message". I'm really sorry... I know from experience how much work doctest upgrades end up taking when you think they will be a 5-minute job.

Changed 8 years ago by roed

Most recent update

comment:5 Changed 8 years ago by roed

  • Status changed from needs_work to needs_review

No problem. I'm not convinced that we need to worry about the __compat function since it's a double underscore method (and does something useful only to coercion), but oh well: I added a deprecation warning instead.

I also updated the commit message and changed Pari to PARI.

comment:6 Changed 8 years ago by kcrisman

  • Cc robertwb added
  • Status changed from needs_review to needs_work
  • Work issues changed from commit message, PARI (maybe?) to raise!=Raise

As I suspected, changing raise to Raise would cause problems.

./sage -b
<snip>
SyntaxError: invalid syntax (element_ext_pari.py, line 653)
Error importing ipy_profile_sage - perhaps you should run %upgrade?
WARNING: Loading of ipy_profile_sage failed.

I don't mind you removing that function! I just don't know what it does or why it can now be removed - actually, upon reading it, it does make sense. If someone else (e.g., robertwb) can verify that the same functionality of checking that two such elements come from the same finite field is in Sage, I am very happy with reducing our doctest needs by removing that.

Changed 8 years ago by roed

Even more recent update. It would be nice to be able to delete patches....

comment:7 Changed 8 years ago by roed

  • Status changed from needs_work to needs_review

Oops. Command C on a Mac capitalizes things; that was not intentional.

I've removed __compat again. I'll find someone else to sign off on it.

Apply 12261.4.patch only.

comment:8 Changed 8 years ago by aly.deines

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Aly Deines
  • Status changed from needs_review to positive_review

comment:9 Changed 8 years ago by kcrisman

  • Work issues raise!=Raise deleted

comment:10 Changed 8 years ago by jdemeyer

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