Opened 6 years ago

Closed 6 years ago

#17862 closed enhancement (fixed)

Remove use of PY_IS_NUMERIC

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.6
Component: c_lib Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 8300f59 (Commits, GitHub, GitLab) Commit: 8300f591ae2987e01d609e856d38758b28c6bf42
Dependencies: #17800 Stopgaps:

Status badges

Description


Change History (14)

comment:1 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/17862

comment:2 Changed 6 years ago by jdemeyer

  • Commit set to 7083f19ecfc83426442aa98c78d50c520337e82e
  • Status changed from new to needs_review

Last 10 new commits:

1e88c72Another polynomial division action test.
a60134ctrac #17740: review 1 (documentation)
728811dtrac #17740: review 2 (clean Errors)
96c1a03trac #17740: review 3 (less in try/except block)
2cb51c0Re-introduce action of fraction field as fallback for division action.
bff474bBetter _pseudo_fraction_field default implementation.
9c970aetrac #17740: merge sage-6.6.beta1
2075e2etrac #17740: avoid parent deaths
4fcee82Merge commit '2075e2e' into t/17800/ticket/17800
7083f19Remove use of PY_IS_NUMERIC

comment:3 Changed 6 years ago by vdelecroix

Could we get rid in the same ticket of _native_coercion_ranks_inv and _native_coercion_ranks and use coerce from Python

>>> coerce(True, 1l)
(1L, 1L)
>>> coerce(complex(0,1), 1.0)
(1j, (1+0j))

Or even better PyNumber_Coerce from the C API.

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

Can you make a new ticket for that instead?

comment:5 in reply to: ↑ 4 Changed 6 years ago by vdelecroix

Replying to jdemeyer:

Can you make a new ticket for that instead?

Yes #17865

comment:6 follow-up: Changed 6 years ago by vdelecroix

I just have two remarks but not for this ticket I guess:

  • Don't we want to use faster functions in py_scalar_to_element?
  • It is clear that we want compatibility between structure.coerce.py_scalar_parent and structure.element.py_scalar_to_element. I do not like the fact that these two functions belong to two different files and do not call each other in their doctests.

comment:7 Changed 6 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix

comment:8 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by jdemeyer

Replying to vdelecroix:

I just have two remarks but not for this ticket I guess:

  • Don't we want to use faster functions in py_scalar_to_element?

If you mean functions like PyFoo_Check(): Cython knows them too, it will generate calls to PyFoo_Check() in isinstance.

  • It is clear that we want compatibility between structure.coerce.py_scalar_parent and structure.element.py_scalar_to_element. I do not like the fact that these two functions belong to two different files and do not call each other in their doctests.

Good point.

comment:9 in reply to: ↑ 8 Changed 6 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

I just have two remarks but not for this ticket I guess:

  • Don't we want to use faster functions in py_scalar_to_element?

If you mean functions like PyFoo_Check(): Cython knows them too, it will generate calls to PyFoo_Check() in isinstance.

Nope. I meant the conversion Python int -> Sage integer and such. The import takes quite a long time and ZZ(x) is suboptimal to create an integer.

Last edited 6 years ago by vdelecroix (previous) (diff)

comment:10 Changed 6 years ago by git

  • Commit changed from 7083f19ecfc83426442aa98c78d50c520337e82e to 7eb0e93b6c74b1e64d11a7ea1ac7fd1cb4c56aa6

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

7eb0e93Move py_scalar_to_element to coerce.pyx

comment:11 Changed 6 years ago by jdemeyer

I moved py_scalar_to_element to coerce.pyx, but I couldn't improve the conversion because of circular import problems.

comment:12 Changed 6 years ago by vdelecroix

  • Branch changed from u/jdemeyer/ticket/17862 to u/vdelecroix/17862
  • Commit changed from 7eb0e93b6c74b1e64d11a7ea1ac7fd1cb4c56aa6 to 8300f591ae2987e01d609e856d38758b28c6bf42

I did not know

sage: isinstance(True,int)
True

Do we really want the bool -> Integer to happen within Sage? We can do an exact type check instead. That would break support for subtyped Python base classes though. Another drawback is that it would not be possible anymore to count like

sage: sum((x >= 3 for x in my_list), ZZ(0))

So I am in favor of your solution.

For the speed-up, that is not dramatic.

I changed the doc to make the link :class:`Element` works. I am ok for positive review.


New commits:

8300f59trac #17862 (review): doc typo

comment:13 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:14 Changed 6 years ago by vbraun

  • Branch changed from u/vdelecroix/17862 to 8300f591ae2987e01d609e856d38758b28c6bf42
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.