Opened 6 years ago
Closed 5 years ago
#22321 closed enhancement (wontfix)
Gen.__init__() does not work as expected
Reported by: | defeo | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | interfaces | Keywords: | PARI, coercion |
Cc: | jdemeyer, vdelecroix | Merged in: | |
Authors: | Reviewers: | Luca De Feo | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/defeo/gen___init_____does_not_work_as_expected (Commits, GitHub, GitLab) | Commit: | e98ea3a1e4b87cad403ae665ff42beadba0b1515 |
Dependencies: | #22470, #22471 | Stopgaps: |
Description (last modified by )
For most types, the following would work, but it does not for Gen
:
sage: t = type(pari(2)) sage: t(2) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-2-35638e9214b8> in <module>() ----> 1 t(Integer(2)) TypeError: __init__() takes exactly 0 positional arguments (1 given)
It makes sense to change this but it will require some refactoring of the new_gen
mechanism.
Change History (12)
comment:1 Changed 6 years ago by
- Description modified (diff)
- Summary changed from pow broken for PARI exponents to Gen.__init__() does not work as expected
- Type changed from PLEASE CHANGE to enhancement
comment:2 follow-up: ↓ 3 Changed 6 years ago by
comment:3 in reply to: ↑ 2 Changed 6 years ago by
Replying to defeo:
Anyway, none of this seems to be strictly needed for #20238, does it?
No. But we should decide what we want to do before the splitting of cypari2
and what after. Fixing Gen.__init__
would make sense before the split. Maybe you should decide when to do this, because you created this ticket (I changed the title because this is why __pow__
didn't work).
comment:4 Changed 6 years ago by
No. But we should decide what we want to do before the splitting of cypari2 and what after. Fixing Gen.init would make sense before the split.
I think we need to discuss this longer. Voice would be preferable.
I changed the title because this is why
__pow__
didn't work
Well, __pow__
doesn't work because it expects Gen
to work in some way, but that expectation is not an absolute necessity. It is indeed a sensible way for a class to behave, though.
comment:5 Changed 6 years ago by
- Branch set to u/defeo/gen___init_____does_not_work_as_expected
comment:6 Changed 6 years ago by
- Commit set to e98ea3a1e4b87cad403ae665ff42beadba0b1515
- Status changed from new to needs_info
This is the best I could come up with. The constructor just calls objtogen
, and then makes a copy of the result.
The copy is unnecessary most of the time, however I don't see how we can avoid it for, e.g. Gen(GF(2)(0))
, i.e. when the argument is an object with a _pari_()
method. Given that you are not allowed to override __new__
, I think we are stuck with this.
New commits:
e98ea3a | cypari2.Gen constructor converts objects to Gen
|
comment:7 follow-up: ↓ 8 Changed 6 years ago by
I guess this would work.
A better way would be to move the code of objtogen
into Gen.__init__
(and then we could remove objtogen
). The _pari_
method would be hard to support without a copy, unless we require that _pari_
needs to return a new object every time (it cannot be cached).
However, I'm not saying that this should be done now.
I was thinking of something else: usually, the Python convention is that special methods have double underscores. Should we replace _pari_
by __pari__
?
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 6 years ago by
A better way would be to move the code of
objtogen
intoGen.__init__
(and then we could removeobjtogen
).
No pickling surprises?
unless we require that
_pari_
needs to return a new object every time (it cannot be cached).
That looks risky.
I was thinking of something else: usually, the Python convention is that special methods have double underscores. Should we replace
_pari_
by__pari__
?
I had the same thought.
comment:9 in reply to: ↑ 8 Changed 6 years ago by
Replying to defeo:
A better way would be to move the code of
objtogen
intoGen.__init__
(and then we could removeobjtogen
).No pickling surprises?
I don't think so. There shouldn't be any difference between the global callable objtogen
and the global callable Gen
.
comment:10 Changed 5 years ago by
- Dependencies changed from #22319 to #22471
comment:11 Changed 5 years ago by
- Dependencies changed from #22471 to #22470, #22471
comment:12 Changed 5 years ago by
- Milestone changed from sage-7.6 to sage-duplicate/invalid/wontfix
- Resolution set to wontfix
- Reviewers set to Luca De Feo
- Status changed from needs_info to closed
Closing as "let's fix this in CyPari2
".
If we keep going along this path,
pari(x)
will be reduntant withGen(x)
. Then we won't needlibs.pari.pari
to be a callable namespace anymore, nor will we need thePariInstance
class. At that point cypari would start resembling an ordinary Python module, with no crazy hacks. Using it would look likeAt that point, it would seem logical to rename
Gen
toPari
, or at least to have such an alias.I like this!
Anyway, none of this seems to be strictly needed for #20238, does it?