Opened 5 years ago

Closed 5 years ago

#22471 closed enhancement (fixed)

Gen: clean up "new_ref" mechanism

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.6
Component: misc Keywords: days85
Cc: defeo, vdelecroix 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 jdemeyer)

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

  • Description modified (diff)

comment:2 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Clean up and document "b" and "refers_to" attributes of Gen to Gen: clean up "new_ref" mechanism

comment:5 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/clean_up_and_document__b__and__refers_to__attributes_of_gen

comment:6 Changed 5 years ago by jdemeyer

  • Commit set to 6b65682846416c83883250b492305588b19c33c0
  • Status changed from new to needs_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 5 years ago by git

  • Commit changed from 6b65682846416c83883250b492305588b19c33c0 to f7bc5b7ea8205e31ef4d98928d289c79c0f9e893

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

f7bc5b7Put sig_check() in inner loops

comment:8 follow-up: Changed 5 years ago by 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?

comment:9 Changed 5 years ago by defeo

The Python3 plugin is failing (one doctest uses cmp)

comment:10 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

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 follow-up: Changed 5 years ago by 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.

comment:12 in reply to: ↑ 11 Changed 5 years ago by defeo

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 follow-up: Changed 5 years ago by jdemeyer

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

  • Dependencies #22193 deleted

comment:15 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by 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? 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 5 years ago by jdemeyer

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

  • Reviewers set to Luca De Feo
  • Status changed from needs_review to positive_review

Nice cleanup. Good to go!

comment:18 Changed 5 years ago by defeo

  • Keywords days85 added

comment:19 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/clean_up_and_document__b__and__refers_to__attributes_of_gen to f7bc5b7ea8205e31ef4d98928d289c79c0f9e893
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.