Bring Doctest coverage for element_ext_pari.py to 100%

[roed314]

Part of metaticket #12024

CC: @robertwb

Component: doctest coverage

Author: David Roe

Reviewer: Karl-Dieter Crisman, Aly Deines

Merged: sage-5.0.beta3

Issue created by migration from https://trac.sagemath.org/ticket/12261

[roed314]

Author: David Roe

[roed314]

comment:1

Attachment: 12261.patch.gz

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:2

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.

[roed314]

Attachment: 12261.2.patch.gz

Apply this patch only

[roed314]

comment:3

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

[kcrisman]

Work Issues: commit message, PARI (maybe?)

[kcrisman]

comment:4

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.

[roed314]

Most recent update

[roed314]

comment:5

Attachment: 12261.3.patch.gz

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.

[kcrisman]

comment:6

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.

[kcrisman]

Changed work issues from commit message, PARI (maybe?) to raise!=Raise

[roed314]

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

[roed314]

comment:7

Attachment: 12261.4.patch.gz

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.

[adeines]

Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Aly Deines

[kcrisman]

Changed work issues from raise!=Raise to none

[jdemeyer]

Merged: sage-5.0.beta3