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 tscrim)

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)

trac_14287-rebased.patch​ (53.8 KB) - added by tscrim 7 years ago.

Download all attachments as: .zip

Change History (34)

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)

comment:15 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

None of the review patches applies properly for me.

comment:16 Changed 7 years ago by tscrim

  • 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 jdemeyer

Doesn't apply for me. Did you keep in mind the dependency #14284?

comment:18 Changed 7 years ago by tscrim

No, I forgot to check when that was merged. Updated.

comment:19 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to positive_review

The patch finally applies now...

comment:20 Changed 7 years ago by tscrim

Thank you for checking Jeroen. I appreciate it.

comment:21 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.9 to sage-5.10

comment:22 Changed 7 years ago by jdemeyer

  • 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 jdemeyer

And also: 

sage -t --long devel/sage/sage/categories/classical_crystals.py  # 1 doctest failed

comment:24 Changed 7 years ago by tscrim

  • Status changed from needs_work to positive_review

Fixed.

comment:25 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

I think this change should be reviewed.

comment:26 Changed 7 years ago by jdemeyer

  • Authors changed from Julian Rueth to Julian Rueth, Travis Scrimshaw
  • Status changed from needs_work to needs_review

comment:27 Changed 7 years ago by roed

  • 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:28 Changed 7 years ago by tscrim

  • Status changed from needs_work to needs_review

Fixed.

comment:29 Changed 7 years ago by roed

  • Status changed from needs_review to positive_review

Looks good.

comment:30 Changed 7 years ago by tscrim

Thanks for re-reviewing the changes.

comment:31 Changed 7 years ago by jdemeyer

  • Dependencies changed from #14284 to #14284, #14278
  • Status changed from positive_review to needs_work

Should be rebased to sage-5.9.beta4.

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

Changed 7 years ago by tscrim

comment:32 Changed 7 years ago by tscrim

  • Status changed from needs_work to positive_review

Rebased patch on 5.9.beta5.

comment:33 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.10.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.