Opened 4 years ago

Closed 3 years ago

#27021 closed enhancement (fixed)

Clean up import structure of coercion_model

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.7
Component: refactoring Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: b61bd81 (Commits, GitHub, GitLab) Commit: b61bd8116ce290e6ad3b728460a6bce5d79ff39d
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Currently, there is a base class CoercionModel defined in element.pyx and the real coercion model is a derived class CoercionModel_cache_maps in coerce.pyx. This is explained by a comment in element.pyx:

# We define this base class here to avoid circular cimports.

However, we can get rid of this circular import by forcing element to be imported first.

Then we can move the coercion model to coerce.pyx. This has the following advantages:

  1. simpler code as the superfluous base class can be deleted.
  1. making more sense as everything regarding the coercion model will be in coerce.pyx
  1. better performance for Cython code that cimports the coercion model as it now has access to al cdef methods and attributes, not only those from the old CoercionModel base class.

Change History (23)

comment:1 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/27021

comment:4 Changed 3 years ago by jdemeyer

  • Commit set to 89343458e8c6d6afcd260ef64c4f87b65e9fe84c
  • Status changed from new to needs_review

New commits:

8934345Clean up import structure of coercion_model

comment:5 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:6 Changed 3 years ago by git

  • Commit changed from 89343458e8c6d6afcd260ef64c4f87b65e9fe84c to bae7f8d91b683ace8eb3d2dddbf69d9bf331e7ed
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

bae7f8dDo not use SageObject for latex

comment:7 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

There is an issue with docbuilding.

comment:8 Changed 3 years ago by git

  • Commit changed from bae7f8d91b683ace8eb3d2dddbf69d9bf331e7ed to 7d17bb40cccfa3c160741c1bdb2432c4166f86b9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7d17bb4Lazy import of latex

comment:9 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:10 Changed 3 years ago by git

  • Commit changed from 7d17bb40cccfa3c160741c1bdb2432c4166f86b9 to e71573dbb0e8bbf86c7f37055dc9109ae3ccdb11

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

e71573dDo not import SageObject in sage.misc.superseded

comment:11 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Patchbots are green.

comment:12 Changed 3 years ago by chapoton

arando is not exactly green...

comment:13 Changed 3 years ago by tscrim

  • Status changed from positive_review to needs_work

When I was looking at it, it looked like it was morally green with some sort of misbuild with giac. However, looking at all of the failures, there are some doctests that indicate an import error from this ticket.

comment:14 Changed 3 years ago by jdemeyer

I suspect (but still need to confirm) that this is just a matter of rebuilding giacpy_sage. The API of element.pxd changed, so any package cimporting that needs to be recompiled.

comment:15 Changed 3 years ago by git

  • Commit changed from e71573dbb0e8bbf86c7f37055dc9109ae3ccdb11 to b61bd8116ce290e6ad3b728460a6bce5d79ff39d

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

b61bd81Force rebuild of giacpy_sage

comment:16 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

A rebuild of giacpy_sage fixes it indeed.

comment:17 Changed 3 years ago by jdemeyer

This again shows why patchbots should install optional packages...

comment:18 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

A real green bot. Thanks.

comment:19 Changed 3 years ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:20 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work
[sagelib-8.6] Error compiling Cython file:
[sagelib-8.6] ------------------------------------------------------------
[sagelib-8.6] ...
[sagelib-8.6] from sage.structure.parent import Parent
[sagelib-8.6] from sage.rings.all import ZZ, QQ, RDF
[sagelib-8.6] 
[sagelib-8.6] from sage.groups.perm_gps.permgroup_element cimport PermutationGroupElement
[sagelib-8.6] from sage.combinat.permutation import Permutation
[sagelib-8.6] from sage.structure.element cimport coercion_model as cm
[sagelib-8.6] ^
[sagelib-8.6] ------------------------------------------------------------
[sagelib-8.6] 
[sagelib-8.6] sage/libs/gap/element.pyx:34:0: 'sage/structure/element/coercion_model.pxd' not found

comment:21 Changed 3 years ago by chapoton

@vbraun: Volker, there was a conflict with #21020, that we fixed by letting #21020 depend on the present ticket. So now you can merge either #27021 only, or both #27021 and the new version of #21020

Last edited 3 years ago by chapoton (previous) (diff)

comment:22 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to positive_review

If you want this to be merged, it should be set back to positive review.

comment:23 Changed 3 years ago by vbraun

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