Opened 8 years ago

Last modified 8 years ago

#14205 needs_info enhancement

polybori doctests involve randomness

Reported by: tkluck Owned by: AlexGhitza
Priority: minor Milestone: sage-wishlist
Component: algebra Keywords:
Cc: AlexanderDreyer Merged in:
Authors: Reviewers:
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

As discusses in #13767, there is some doctests for the random_set function in polybori that have inherent random results. Because of that, they might break if the implementation of random functions changes, such as what happened in that ticket.

This is a rare occasion, but still it might be worthwhile to change these kind of doctests to only check for documented behavior.

For example, the doctests could check that result = random_set(monomial, n) should obey len(result) == n and result.diff(monomial.divisors()).empty(). (suggested by Alexander Dreyer)

Or we could test against theorems (such as: One has random data, but the test is using an identity that must hold for any data). (suggested by Simon King)

Change History (7)

comment:1 follow-up: Changed 8 years ago by vbraun

I'd rather have the output of boost.Random doctested so we know when the implementation changes. We also test all other RNGs to verify that the sequence that they produce is actually the same, that is, we set the seed correctly.

comment:2 in reply to: ↑ 1 Changed 8 years ago by AlexanderDreyer

Replying to vbraun:

I'd rather have the output of boost.Random doctested so we know when the implementation changes. We also test all other RNGs to verify that the sequence that they produce is actually the same, that is, we set the seed correctly.

Well, I understood from the discussion here, that the test should not depend on the random generator: https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/uSEWZrE-T_8

Also, sage-on-gentoo and sage-on-debian may use different versions of boost than the original Sage.

An alternative could be that we additionally add (and document) the generic tests and keep the explicit ones. This would simplify the bug hunt next time.

comment:3 Changed 8 years ago by AlexanderDreyer

  • Report Upstream changed from N/A to None of the above - read trac for reasoning.

comment:4 Changed 8 years ago by vbraun

  • Status changed from new to needs_review

Doctesting the random sequence (see sage.misc.randstate) and doctesting the correctness of the output are two different things. And ideally we'd have both, though its better to have randstate-dependent tests than none at all.

comment:5 Changed 8 years ago by vbraun

  • Milestone changed from sage-5.8 to sage-wishlist

comment:6 Changed 8 years ago by AlexanderDreyer

  • Status changed from needs_review to needs_info

So you would just keep it, like it was in #13767?

comment:7 Changed 8 years ago by vbraun

Correct me if I'm wrong, but the failing doctests were pretty much directly tests of the random number generator. If thats the case then I'd keep them since they test the rng seeding.

Note: See TracTickets for help on using tickets.