Opened 6 years ago
Closed 5 years ago
#21059 closed defect (fixed)
Change WithRealizations._an_element_ to use a_realization
| Reported by: | tscrim | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | sage-7.3 |
| Component: | categories | Keywords: | days85 |
| Cc: | ntheiry, darij | Merged in: | |
| Authors: | Travis Scrimshaw | Reviewers: | Florent Hivert |
| Report Upstream: | N/A | Work issues: | |
| Branch: | f8a1bbb (Commits, GitHub, GitLab) | Commit: | f8a1bbb2cade40a8d21cc43d35db6ec5bd1c922f |
| Dependencies: | Stopgaps: |
Status badges
Description
As noticed on #21054, there is a discrepancy between one and _an_element_ for parents that are in the category *.WithRealizations, in that the former uses a_realization, whereas the latter uses realizations()[0]. While on #21054, this did uncover an error, this cases problems with the test suite when a realization does not get created before an_element gets called (which I have noticed before).
Thus, I propose to have _an_element_ use a_realization(), which is a required implementation and _an_element_ will also have consistent output no matter which basis is created first.
Change History (7)
comment:1 Changed 6 years ago by
- Branch set to public/categories/change_an_element_with_realizations-21059
- Commit set to f8a1bbb2cade40a8d21cc43d35db6ec5bd1c922f
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
There are two failing doc-tests in sage/combinat/kazhdan_lusztig when the optional coxeter3 package is installed.
1 item had failures:
2 of 8 in sage.combinat.kazhdan_lusztig.KazhdanLusztigPolynomial
[26 tests, 2 failures, 2.76 s]
----------------------------------------------------------------------
sage -t kazhdan_lusztig.py # 2 doctests failed
----------------------------------------------------------------------
and more failures in sage/libs/coxeter3/.
---------------------------------------------------------------------- sage -t coxeter3/coxeter.pyx # 256 doctests failed sage -t coxeter3/coxeter_group.py # 120 doctests failed ----------------------------------------------------------------------
Not entirely sure what the policy is on doc-tests in optional packages...
comment:3 Changed 6 years ago by
Are these are all fixed by #21077?
comment:4 Changed 5 years ago by
Except for rebasing and doctests this looks good to me. I'll handle the rebasing shortly and put a positive review.
comment:5 Changed 5 years ago by
- Reviewers set to Florent Hivert
- Status changed from needs_review to positive_review
comment:6 Changed 5 years ago by
- Keywords days85 added
comment:7 Changed 5 years ago by
- Branch changed from public/categories/change_an_element_with_realizations-21059 to f8a1bbb2cade40a8d21cc43d35db6ec5bd1c922f
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Use a_realization() instead of realizations()[0].