Opened 7 years ago
Closed 7 years ago
#14287 closed enhancement (fixed)
Split _test_elements_eq
Reported by: | saraedum | Owned by: | nthiery |
---|---|---|---|
Priority: | trivial | Milestone: | sage-5.10 |
Component: | categories | Keywords: | |
Cc: | roed | Merged in: | sage-5.10.beta0 |
Authors: | Julian Rueth, Travis Scrimshaw | Reviewers: | David Roe |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14284, #14278 | Stopgaps: |
Description (last modified by )
Currently _test_elements_eq
tests for reflexivity, symmetry, and transitivity. However, transitivity does not hold in padic rings. This tickets splits
_test_elements_eq
into these three subtests, so they that we can override the transitivity test for padic implementations.
Apply:
Attachments (1)
Change History (34)
comment:1 Changed 7 years ago by
- Dependencies set to #12484
comment:2 Changed 7 years ago by
- Dependencies changed from #12484 to #14284
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
comment:5 Changed 7 years ago by
- Description modified (diff)
comment:6 Changed 7 years ago by
apply trac_14287.patch 14287_review_58rc0.patch
comment:7 Changed 7 years ago by
apply trac_14287.patch 14287_review.patch
comment:8 Changed 7 years ago by
- Description modified (diff)
comment:9 Changed 7 years ago by
- Reviewers set to David Roe
- Status changed from needs_review to positive_review
Tests pass; looks good.
comment:10 Changed 7 years ago by
- Milestone changed from sage-5.9 to sage-pending
comment:11 follow-up: ↓ 12 Changed 7 years ago by
Just a micro comment about the use of this ticket; not its implementation.
However, transitivity does not hold in padic rings
Sounds scary! Well, as long as you know what you are doing :-)
so they that we can override the transitivity test for padic
implementations
I am not sure what's your plan for this; just in case, and unless this becomes too cumbersome, I prefer writing explicitly each time:
sage: TestSuite(...).run(skip="....transitivity")
rather than systematically disabling the transitivity test. First, that's the occasion to warn the user about an atypical behavior. And to get more people to thing about creative ways of fixing that. Second, the test might actually pass in certain cases, and it's a useful information to record.
Cheers,
Nicolas
comment:12 in reply to: ↑ 11 Changed 7 years ago by
I see your point. However, these tests will not pass and there is no way to avoid this (other than disabling ==
completely). It's an intrinsic problem with padics (or anything that is stored to different precision).
I agree that we could use a skip
to warn the user but on the other hand it is probably more confusing...
comment:13 Changed 7 years ago by
- Milestone changed from sage-pending to sage-5.9
comment:14 Changed 7 years ago by
- Description modified (diff)
comment:15 Changed 7 years ago by
- Status changed from positive_review to needs_work
None of the review patches applies properly for me.
comment:16 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
I've uploaded a rebased patch to at least 5.9.beta1
and has the review patch folded in. Since this touches a lot of files, I'd like someone with a more recent beta version to make sure it applies before setting this back to positive review.
For patchbot:
Apply: trac_14287-rebased.patch
comment:17 Changed 7 years ago by
Doesn't apply for me. Did you keep in mind the dependency #14284?
comment:18 Changed 7 years ago by
No, I forgot to check when that was merged. Updated.
comment:19 Changed 7 years ago by
- Status changed from needs_review to positive_review
The patch finally applies now...
comment:20 Changed 7 years ago by
Thank you for checking Jeroen. I appreciate it.
comment:21 Changed 7 years ago by
- Milestone changed from sage-5.9 to sage-5.10
comment:22 Changed 7 years ago by
- Status changed from positive_review to needs_work
sage -t --long devel/sage/doc/en/thematic_tutorials/coercion_and_categories.rst # 5 doctests failed
comment:23 Changed 7 years ago by
And also:
sage -t --long devel/sage/sage/categories/classical_crystals.py # 1 doctest failed
comment:25 Changed 7 years ago by
- Status changed from positive_review to needs_work
I think this change should be reviewed.
comment:26 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:27 Changed 7 years ago by
- Status changed from needs_review to needs_work
The last hunk of classical_crystals.py
removes a newline before a ::
. Otherwise the changes are good.
comment:30 Changed 7 years ago by
Thanks for re-reviewing the changes.
comment:31 Changed 7 years ago by
- Dependencies changed from #14284 to #14284, #14278
- Status changed from positive_review to needs_work
Should be rebased to sage-5.9.beta2.
Changed 7 years ago by
comment:32 Changed 7 years ago by
- Status changed from needs_work to positive_review
Rebased patch on 5.9.beta5
.
comment:33 Changed 7 years ago by
- Merged in set to sage-5.10.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
apply trac_14287.patch 14287_review_58rc0.patch