Opened 6 years ago

Closed 6 years ago

#22471 closed enhancement (fixed)

Gen: clean up "new_ref" mechanism

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

Status badges

Description (last modified by Jeroen Demeyer)

I have always found it hard to understand the new_ref mechanism of Gen and what the related attributes b and refers_to mean. I propose:

  1. Rename b to chunck.
  1. Split refers_to in two because it is used in two independent ways: instead add parent and itemcache attributes.
  1. Turn new_ref into a method of Gen.
  1. Document this a lot better.

Change History (19)

comment:1 Changed 6 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 6 years ago by Jeroen Demeyer

Description: modified (diff)

comment:3 Changed 6 years ago by Jeroen Demeyer

Description: modified (diff)

comment:4 Changed 6 years ago by Jeroen Demeyer

Description: modified (diff)
Summary: Clean up and document "b" and "refers_to" attributes of GenGen: clean up "new_ref" mechanism

comment:5 Changed 6 years ago by Jeroen Demeyer

Branch: u/jdemeyer/clean_up_and_document__b__and__refers_to__attributes_of_gen

comment:6 Changed 6 years ago by Jeroen Demeyer

Commit: 6b65682846416c83883250b492305588b19c33c0
Status: newneeds_review

New commits:

5765a58Made docstrings in libs/cypari2 mostly Sage-independent
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
6b65682Gen: clean up "new_ref" mechanism

comment:7 Changed 6 years ago by git

Commit: 6b65682846416c83883250b492305588b19c33c0f7bc5b7ea8205e31ef4d98928d289c79c0f9e893

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

f7bc5b7Put sig_check() in inner loops

comment:8 Changed 6 years ago by Luca De Feo

Since this is moving new_ref, I'll take the occasion to ask whether _new_ref wouldn't be a better name. Isn't it the case that this is exclusively for internal purposes?

comment:9 Changed 6 years ago by Luca De Feo

The Python3 plugin is failing (one doctest uses cmp)

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

Replying to defeo:

Since this is moving new_ref, I'll take the occasion to ask whether _new_ref wouldn't be a better name. Isn't it the case that this is exclusively for internal purposes?

I would argue that it's part of the public Cython API (for example, it's used in the new power_series_pari.pyx), so it doesn't need to be private. Note that it's a cdef method, so it will not bother TAB-completion.

comment:11 Changed 6 years ago by Jeroen Demeyer

That being said, I want my choose my battles wisely and focus on method name bikeshedding in #22470 :-)

So if you insist on _new_ref, I will gladly make the change.

comment:12 in reply to:  11 Changed 6 years ago by Luca De Feo

Replying to jdemeyer:

That being said, I want my choose my battles wisely and focus on method name bikeshedding in #22470 :-)

So if you insist on _new_ref, I will gladly make the change.

No, no. The argument on new_ref being used by power series is a good one. I hadn't read the diff that far.

The Py3 failure must be dealt with, however.

comment:13 Changed 6 years ago by Jeroen Demeyer

About the py3 failure:

This is OK since Cython understands xrange:

            inds = xrange(*n.indices(l))

This comes from the dependency which is merged now (there is no cmp in the current diff):

            sage: cmp(pari('2/3'), pari('2/5'))

So there is nothing to be done here...

comment:14 Changed 6 years ago by Jeroen Demeyer

Dependencies: #22193

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

Replying to jdemeyer:

About the py3 failure:

This is OK since Cython understands xrange:

            inds = xrange(*n.indices(l))

Mmm... right. But is it going to support it in the long run? I cannot find much info on what Cython plans are. What's the most future proof way of using iterators in Cython?

This comes from the dependency which is merged now (there is no cmp in the current diff):

            sage: cmp(pari('2/3'), pari('2/5'))

So there is nothing to be done here...

Right. That'll be for another ticket.

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

Replying to defeo:

Replying to jdemeyer:

About the py3 failure:

This is OK since Cython understands xrange:

            inds = xrange(*n.indices(l))

Mmm... right. But is it going to support it in the long run?

Why should they not? It's not deprecated or anything...

comment:17 Changed 6 years ago by Luca De Feo

Reviewers: Luca De Feo
Status: needs_reviewpositive_review

Nice cleanup. Good to go!

comment:18 Changed 6 years ago by Luca De Feo

Keywords: days85 added

comment:19 Changed 6 years ago by Volker Braun

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