Replace __getslice__ with functionality in __getitem__ in several files (part 2)

[a-andre]

__getslice__ has been deprecated for a long time in Python. This patch adds equivalent functionality to __getitem__, which is where the functionality should be.

See #12041 comment:4.

Apply:

• attachment: trac_12093.rebased.patch

CC: @kiwifb @jasongrout

Component: build

Author: André Apitzsch

Reviewer: François Bissey, David Loeffler

Merged: sage-5.0.beta9

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

[a-andre]

comment:1

Attachment: trac_12093.patch.gz

Currently there is one failing test in "devel/sage/sage/rings/finite_rings/homset.py".

sage: H = Hom(GF(32, 'a'), GF(1024, 'b'))
sage: type(H[2:4])    # w/o patch
<class 'sage.structure.sequence.Sequence_generic'>
sage: type(H[2:4])    # w/ patch
<type 'list'>

Suggestions to solve this problem are welcome.

[kiwifb]

comment:2

What are in both case the types of "H" and say "H[1]"? I am actually not to worried here, I don't think the individual elements of H have ever been of type sage.structure.sequence.Sequence_generic I am suspecting that's just a type for the slice. In this context a list is probably just as valid. We should check what the test is supposed to "test", but it may just a test that slicing is working.

Just looked at the file before posting, what is the actual output of the test?

[a-andre]

comment:3

In both cases the same types.

sage: H = Hom(GF(32, 'a'), GF(1024, 'b'))
sage: type(H[1])
<class 'sage.rings.finite_rings.homset.FiniteFieldHomomorphism_im_gens'>
sage: type(H)
<class 'sage.rings.finite_rings.homset.FiniteFieldHomset_with_category'>

Output of the test sage: H[2:4]

without patch:

[
Ring morphism:
  From: Finite Field in a of size 2^5
  To:   Finite Field in b of size 2^10
  Defn: a |--> b^8 + b^6 + b^2,
Ring morphism:
  From: Finite Field in a of size 2^5
  To:   Finite Field in b of size 2^10
  Defn: a |--> b^9 + b^7 + b^6 + b^5 + b^4
]

with patch:

[Ring morphism:
  From: Finite Field in a of size 2^5
  To:   Finite Field in b of size 2^10
  Defn: a |--> b^8 + b^6 + b^2, Ring morphism:
  From: Finite Field in a of size 2^5
  To:   Finite Field in b of size 2^10
  Defn: a |--> b^9 + b^7 + b^6 + b^5 + b^4]

[kiwifb]

comment:4

Seems like a fairly reasonable output to me does the doctest complains about formatting? If it is just that, I would just add a patch to take into account the new format.

I may be able to test this a little bit later.

[a-andre]

Apply after trac_12093.patch

[a-andre]

comment:5

Attachment: trac_12093_2.patch.gz

With the additional patch attachment: trac_12093_2.patch all tests passed.

[kiwifb]

comment:6

That's interesting but you re-introduce some *slice. Do you know why/where they are needed?

[a-andre]

comment:7

list is a subclass of Sequence_generic hence Sequence_generic inherit __*slice__ from list. I have to overwrite these __*slice__ functions as long as Sage uses python 2.* otherwise the __*slice__ functions of list are called.

[kiwifb]

comment:9

Replying to @a-andre:

list is a subclass of Sequence_generic hence Sequence_generic inherit __*slice__ from list. I have to overwrite these __*slice__ functions as long as Sage uses python 2.* otherwise the __*slice__ functions of list are called.

An unfortunate fact but you are correct. At least the code for it is minimal and will be easily removable when needed. Could you please add a comment documenting what you said just before {{{getslice}} in the code? That would be very helpful for future reference.

[a-andre]

Attachment: trac_12093_comment.patch.gz

Apply after trac_12093_2.patch

[a-andre]

comment:10

Replying to @kiwifb:

Could you please add a comment documenting what you said just before {{{getslice}} in the code? That would be very helpful for future reference.

Done.

[kiwifb]

comment:11

Sorry I would liked to review this but I am quite involved with #9958 and I got some issues that I want to sort with #4260, so this is taking the back seat. Do you want to review Jason?

[jasongrout]

comment:12

Where are we in the review? Does it all need a review, or do I need to check something at the end? (It's the last week of classes, so I'm kind of busy with end-of-semester things right now...)

[kiwifb]

comment:13

I guess I am happy with the code and I already asked for extra stuff but it needs to be tested. I don't think it needs testing across a variety of platforms it either works or fails. André has already done some basic testing himself it needs crosschecking.

[kiwifb]

Reviewer: François Bissey

[kiwifb]

comment:14

I am giving this a positive review. I am happy with the code as it is and it does everything that is possible at the current time for these instances of *slice.

[jdemeyer]

comment:16

This should be rebased against #9958 + #9138 + #11900 (all included in sage-5.0.prealpha1)

[jdemeyer]

Work Issues: rebase

[a-andre]

based on sage-5.0.prealpha1

[a-andre]

comment:17

Attachment: trac_12093.rebased.patch.gz

I attached a rebased version and additionally changed the raise syntax preparing for Python 3 (see PEP 3109). Hope that's okay.

[a-andre]

Changed work issues from rebase to none

[loefflerd]

comment:18

Apply trac_12093.rebased.patch

(for the patchbot)

[loefflerd]

Changed reviewer from François Bissey to François Bissey, David Loeffler

[loefflerd]

comment:19

Looks good to me, and the patchbot seems happy too. Positive review.

[jdemeyer]

Merged: sage-5.0.beta9