Opened 4 years ago

Closed 4 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:

Status badges

Description (last modified by jdemeyer)

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 4 years ago by jdemeyer

  • 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: Changed 4 years ago by defeo

If we keep going along this path, pari(x) will be reduntant with Gen(x). Then we won't need libs.pari.pari to be a callable namespace anymore, nor will we need the PariInstance class. At that point cypari would start resembling an ordinary Python module, with no crazy hacks. Using it would look like

>>> from cypari import *
>>> x = Gen('x')
>>> p = x**2 + 3
>>> K = nfinit(p)
>>> K == p.nfinit()
True

At that point, it would seem logical to rename Gen to Pari, or at least to have such an alias.

I like this!

Anyway, none of this seems to be strictly needed for #20238, does it?

comment:3 in reply to: ↑ 2 Changed 4 years ago by jdemeyer

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 4 years ago by defeo

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 4 years ago by defeo

  • Branch set to u/defeo/gen___init_____does_not_work_as_expected

comment:6 Changed 4 years ago by defeo

  • Authors set to Luca De Feo
  • 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:

e98ea3acypari2.Gen constructor converts objects to Gen

comment:7 follow-up: Changed 4 years ago by jdemeyer

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: Changed 4 years ago by defeo

A better way would be to move the code of objtogen into Gen.__init__ (and then we could remove objtogen).

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 4 years ago by jdemeyer

Replying to defeo:

A better way would be to move the code of objtogen into Gen.__init__ (and then we could remove objtogen).

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 4 years ago by jdemeyer

  • Dependencies changed from #22319 to #22471

comment:11 Changed 4 years ago by jdemeyer

  • Dependencies changed from #22471 to #22470, #22471

comment:12 Changed 4 years ago by jdemeyer

  • Authors Luca De Feo deleted
  • 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".

Note: See TracTickets for help on using tickets.