Opened 6 years ago
Closed 6 years ago
#22470 closed enhancement (fixed)
Replace _pari_ > __pari__
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  major  Milestone:  sage7.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: 
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 followup: 6 Changed 6 years ago by
comment:2 followup: 3 Changed 6 years ago by
Aren't those doubleunderscore identifiers reserved for use by Python itself?
comment:3 followup: 5 Changed 6 years ago by
Replying to mmezzarobba:
Aren't those doubleunderscore 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#reservedclassesofidentifiers but again it's not really clear.
comment:4 Changed 6 years ago by
In order to defend __pari__
: that's also the style used by NumPy, which defines special methods like __array__
.
comment:5 followup: 7 Changed 6 years ago by
Replying to jdemeyer:
Replying to mmezzarobba:
Aren't those doubleunderscore identifiers reserved for use by Python itself?
Do you have a reference for that?
https://docs.python.org/2/reference/lexical_analysis.html#reservedclassesofidentifiers says:
__*__
Systemdefined 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 Changed 6 years ago by
Replying to vdelecroix:
One reason for double underscore is that they correspond to actual implementation of special Python syntax
a.__len__()
islen(a)
a.__add__(b)
isa + 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 Changed 6 years ago by
Let's look at our options:
_pari_()
: not documented/used anywhere except for Sage.
_pari()
: meant for private methods, which is not correct here. We want this method to be part of the CyPari2 public API.
__pari()
: even more private, so even less applicable.
__pari__()
: consistent with Python and NumPy. The only issue is that creating such names possibly goes against the Python documentation.
pari()
: very simple but it doesn't make it clear that it has a special meaning. Higher chance of false positives with people using apari()
method for something else.
comment:8 followup: 9 Changed 6 years ago by
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 sagedevel?
comment:9 Changed 6 years ago by
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 sagedevel?
I could try...
comment:10 Changed 6 years ago by
Branch:  → u/jdemeyer/ticket/22470 

comment:11 Changed 6 years ago by
Commit:  → c7afe4918bc026ad99ffee8d475098cecb9e87c1 

Status:  new → needs_review 
Email to sagedevel
sent...
Last 10 new commits:
1a3bc27  Removed Sagespecific bits from guide to real precision

a397d9f  Add sage.libs.pari to the documentation

a44a1a9  Restored some doctests

4a92c0e  Removed one unnecessary conversion to int in doctest

3a382ba  Restored old example with bnr

9f6f4ec  Fixed doctests failing on 32 bits, and improved docs

fb79fd9  Update documentation about precision

98b01fd  Replace _pari_ > __pari__

dc949f3  Remove unused code

c7afe49  Remove some superfluous calls to __pari__()

comment:12 Changed 6 years ago by
Commit:  c7afe4918bc026ad99ffee8d475098cecb9e87c1 → e159033dd1fad71a35078a010e45231120444066 

Branch pushed to git repo; I updated commit sha1. New commits:
e159033  Support _pari_ as deprecated alias of __pari__

comment:13 followup: 15 Changed 6 years ago by
On my sagedevel post, I don't see much objections to __pari__
. But people are stressing backwardscompatibility, so I will take care of that.
comment:14 Changed 6 years ago by
Commit:  e159033dd1fad71a35078a010e45231120444066 → 0279ad83f16822619933f7d3f990cfe2a871ceee 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0279ad8  Support _pari_ as deprecated alias of __pari__

comment:15 followup: 16 Changed 6 years ago by
Replying to jdemeyer:
On my sagedevel 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 Changed 6 years ago by
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
Commit:  0279ad83f16822619933f7d3f990cfe2a871ceee → ab2c895b5e8a817ce6670ecdfedf79ce20390a87 

Branch pushed to git repo; I updated commit sha1. New commits:
ab2c895  Merge tag '7.6.rc0' into t/22470/ticket/22470

comment:18 Changed 6 years ago by
Dependencies:  #22193 

Reviewers:  → Luca De Feo 
Status:  needs_review → positive_review 
After a lenghty discussion in https://groups.google.com/forum/#!topic/sagedevel/G1TuFbh0dA, this ticket wins!
comment:19 Changed 6 years ago by
Branch:  u/jdemeyer/ticket/22470 → ab2c895b5e8a817ce6670ecdfedf79ce20390a87 

Resolution:  → fixed 
Status:  positive_review → closed 
One reason for double underscore is that they correspond to actual implementation of special Python syntax
a.__len__()
islen(a)
a.__add__(b)
isa + b
Do you have an actual reference for why we have to switch fron one to two underscores?