Opened 7 years ago

Last modified 7 years ago

#14287 closed enhancement

Split _test_elements_eq — at Version 14

Reported by: saraedum Owned by: nthiery
Priority: trivial Milestone: sage-5.10
Component: categories Keywords:
Cc: roed Merged in:
Authors: Julian Rueth Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14284 Stopgaps:

Description (last modified by jdemeyer)

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

  1. trac_14287.patch
  2. 14287_review_58rc0.patch

Change History (14)

comment:1 Changed 7 years ago by saraedum

  • Dependencies set to #12484

comment:2 Changed 7 years ago by saraedum

  • Dependencies changed from #12484 to #14284
  • Status changed from new to needs_review

comment:3 Changed 7 years ago by saraedum

  • Description modified (diff)

comment:4 Changed 7 years ago by saraedum

apply trac_14287.patch 14287_review_58rc0.patch

Last edited 7 years ago by saraedum (previous) (diff)

comment:5 Changed 7 years ago by saraedum

  • Description modified (diff)

comment:6 Changed 7 years ago by saraedum

apply trac_14287.patch 14287_review_58rc0.patch

comment:7 Changed 7 years ago by saraedum

apply trac_14287.patch 14287_review.patch

comment:8 Changed 7 years ago by saraedum

  • Description modified (diff)

comment:9 Changed 7 years ago by roed

  • Reviewers set to David Roe
  • Status changed from needs_review to positive_review

Tests pass; looks good.

comment:10 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.9 to sage-pending

comment:11 follow-up: Changed 7 years ago by nthiery

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 saraedum

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 jdemeyer

  • Milestone changed from sage-pending to sage-5.9

comment:14 Changed 7 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.