Opened 6 years ago

Closed 6 years ago

#22470 closed enhancement (fixed)

Replace _pari_ -> __pari__

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-7.6
Component: interfaces Keywords:
Cc: Luca De Feo, Vincent Delecroix Merged in:
Authors: Jeroen Demeyer Reviewers: Luca De Feo
Report Upstream: N/A Work issues:
Branch: ab2c895 (Commits, GitHub, GitLab) Commit: ab2c895b5e8a817ce6670ecdfedf79ce20390a87
Dependencies: Stopgaps:

Status badges

Description

Special methods in Python usually have double leading and trailing underscores. For this reason, it makes sense to use __pari__ for conversion to CyPari2.

Change History (19)

comment:1 Changed 6 years ago by Vincent Delecroix

One reason for double underscore is that they correspond to actual implementation of special Python syntax

  • a.__len__() is len(a)
  • a.__add__(b) is a + b
  • etc

Do you have an actual reference for why we have to switch fron one to two underscores?

comment:2 Changed 6 years ago by Marc Mezzarobba

Aren't those double-underscore identifiers reserved for use by Python itself?

comment:3 in reply to:  2 ; Changed 6 years ago by Jeroen Demeyer

Replying to mmezzarobba:

Aren't those double-underscore identifiers reserved for use by Python itself?

Do you have a reference for that? There is something about underscores in PEP 8 but, after reading it, I still don't know which underscore style to use. And the Sage _foo_ style doesn't even exist for PEP 8.

There is some more in https://docs.python.org/3/reference/lexical_analysis.html#reserved-classes-of-identifiers but again it's not really clear.

Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:4 Changed 6 years ago by Jeroen Demeyer

In order to defend __pari__: that's also the style used by NumPy, which defines special methods like __array__.

comment:5 in reply to:  3 ; Changed 6 years ago by Marc Mezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

Aren't those double-underscore identifiers reserved for use by Python itself?

Do you have a reference for that?

https://docs.python.org/2/reference/lexical_analysis.html#reserved-classes-of-identifiers says:

__*__

System-defined names. These names are defined by the interpreter and its implementation (including the standard library). Current system names are discussed in the Special method names section and elsewhere. More will likely be defined in future versions of Python. Any use of * names, in any context, that does not follow explicitly documented use, is subject to breakage without warning.

comment:6 in reply to:  1 Changed 6 years ago by Jeroen Demeyer

Replying to vdelecroix:

One reason for double underscore is that they correspond to actual implementation of special Python syntax

  • a.__len__() is len(a)
  • a.__add__(b) is a + b
  • etc

One could argue that x.__pari__() is pari(x) that's not really relevant.

Special methods in Python use the __foo__ underscore style. I consider __pari__ to be very similar in spirit to Python's __int__ or NumPy's __array__. Since no PEP or Python documentation documents how to name custom special methods, I think that __pari__ makes the most sense.

comment:7 in reply to:  5 Changed 6 years ago by Jeroen Demeyer

Let's look at our options:

  1. _pari_(): not documented/used anywhere except for Sage.
  1. _pari(): meant for private methods, which is not correct here. We want this method to be part of the CyPari2 public API.
  1. __pari(): even more private, so even less applicable.
  1. __pari__(): consistent with Python and NumPy. The only issue is that creating such names possibly goes against the Python documentation.
  1. pari(): very simple but it doesn't make it clear that it has a special meaning. Higher chance of false positives with people using a pari() method for something else.

comment:8 Changed 6 years ago by Vincent Delecroix

for me it the match is between 2 and 4. But it concerns more than just pari (e.g. magma, maxima, gap and such). What about having the discussion on sage-devel?

comment:9 in reply to:  8 Changed 6 years ago by Jeroen Demeyer

Replying to vdelecroix:

for me it the match is between 2 and 4.

My first choice is 4, my second choice would be 5.

But it concerns more than just pari (e.g. magma, maxima, gap and such).

Right, but we do not have to change everything at the same time. We can do this for each interface separately. I just wanted to do this for _pari_ now because of CyPari2.

What about having the discussion on sage-devel?

I could try...

Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:10 Changed 6 years ago by Jeroen Demeyer

Branch: u/jdemeyer/ticket/22470

comment:11 Changed 6 years ago by Jeroen Demeyer

Commit: c7afe4918bc026ad99ffee8d475098cecb9e87c1
Status: newneeds_review

E-mail to sage-devel sent...


Last 10 new commits:

1a3bc27Removed Sage-specific bits from guide to real precision
a397d9fAdd sage.libs.pari to the documentation
a44a1a9Restored some doctests
4a92c0eRemoved one unnecessary conversion to int in doctest
3a382baRestored old example with bnr
9f6f4ecFixed doctests failing on 32 bits, and improved docs
fb79fd9Update documentation about precision
98b01fdReplace _pari_ -> __pari__
dc949f3Remove unused code
c7afe49Remove some superfluous calls to __pari__()

comment:12 Changed 6 years ago by git

Commit: c7afe4918bc026ad99ffee8d475098cecb9e87c1e159033dd1fad71a35078a010e45231120444066

Branch pushed to git repo; I updated commit sha1. New commits:

e159033Support _pari_ as deprecated alias of __pari__

comment:13 Changed 6 years ago by Jeroen Demeyer

On my sage-devel post, I don't see much objections to __pari__. But people are stressing backwards-compatibility, so I will take care of that.

comment:14 Changed 6 years ago by git

Commit: e159033dd1fad71a35078a010e452311204440660279ad83f16822619933f7d3f990cfe2a871ceee

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0279ad8Support _pari_ as deprecated alias of __pari__

comment:15 in reply to:  13 ; Changed 6 years ago by Luca De Feo

Replying to jdemeyer:

On my sage-devel post, I don't see much objections to __pari__.

There is some though. I always find it hard to make a decision in these circumstances. I'm in favor of the change, but what should be considered as sufficient consensus?

comment:16 in reply to:  15 Changed 6 years ago by Jeroen Demeyer

Replying to defeo:

what should be considered as sufficient consensus?

I don't feel like I should answer this question, since I am too much biased in favour of __pari__.

At least the objections about backwards compatibility are taken care of with the last commit.

comment:17 Changed 6 years ago by git

Commit: 0279ad83f16822619933f7d3f990cfe2a871ceeeab2c895b5e8a817ce6670ecdfedf79ce20390a87

Branch pushed to git repo; I updated commit sha1. New commits:

ab2c895Merge tag '7.6.rc0' into t/22470/ticket/22470

comment:18 Changed 6 years ago by Luca De Feo

Dependencies: #22193
Reviewers: Luca De Feo
Status: needs_reviewpositive_review

After a lenghty discussion in https://groups.google.com/forum/#!topic/sage-devel/G1TuFbh0d-A, this ticket wins!

comment:19 Changed 6 years ago by Volker Braun

Branch: u/jdemeyer/ticket/22470ab2c895b5e8a817ce6670ecdfedf79ce20390a87
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.