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: |
Description (last modified by )
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:
- simpler code as the superfluous base class can be deleted.
- making more sense as everything regarding the coercion model will be in
coerce.pyx
- better performance for Cython code that
cimport
s the coercion model as it now has access to alcdef
methods and attributes, not only those from the oldCoercionModel
base class.
Change History (23)
comment:1 Changed 3 years ago by
- Description modified (diff)
comment:2 Changed 3 years ago by
- Description modified (diff)
comment:3 Changed 3 years ago by
- Branch set to u/jdemeyer/ticket/27021
comment:4 Changed 3 years ago by
- Commit set to 89343458e8c6d6afcd260ef64c4f87b65e9fe84c
- Status changed from new to needs_review
comment:5 Changed 3 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM.
comment:6 Changed 3 years ago by
- 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:
bae7f8d | Do not use SageObject for latex
|
comment:7 Changed 3 years ago by
- Status changed from needs_review to needs_work
There is an issue with docbuilding.
comment:8 Changed 3 years ago by
- Commit changed from bae7f8d91b683ace8eb3d2dddbf69d9bf331e7ed to 7d17bb40cccfa3c160741c1bdb2432c4166f86b9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7d17bb4 | Lazy import of latex
|
comment:9 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:10 Changed 3 years ago by
- Commit changed from 7d17bb40cccfa3c160741c1bdb2432c4166f86b9 to e71573dbb0e8bbf86c7f37055dc9109ae3ccdb11
Branch pushed to git repo; I updated commit sha1. New commits:
e71573d | Do not import SageObject in sage.misc.superseded
|
comment:11 Changed 3 years ago by
- Status changed from needs_review to positive_review
Patchbots are green.
comment:12 Changed 3 years ago by
arando is not exactly green...
comment:13 Changed 3 years ago by
- 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
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
- Commit changed from e71573dbb0e8bbf86c7f37055dc9109ae3ccdb11 to b61bd8116ce290e6ad3b728460a6bce5d79ff39d
Branch pushed to git repo; I updated commit sha1. New commits:
b61bd81 | Force rebuild of giacpy_sage
|
comment:16 Changed 3 years ago by
- Status changed from needs_work to needs_review
A rebuild of giacpy_sage
fixes it indeed.
comment:17 Changed 3 years ago by
This again shows why patchbots should install optional packages...
comment:18 Changed 3 years ago by
- Status changed from needs_review to positive_review
A real green bot. Thanks.
comment:19 Changed 3 years ago by
- 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
- 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
comment:22 Changed 3 years ago by
- 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
- Branch changed from u/jdemeyer/ticket/27021 to b61bd8116ce290e6ad3b728460a6bce5d79ff39d
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Clean up import structure of coercion_model