Opened 2 years ago

Closed 2 years ago

#29905 closed enhancement (fixed)

test basic properties of polyhedra

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.2
Component: geometry Keywords: polyhedra, test suite
Cc: jipilab, gh-LaisRast, dimpase, mjo Merged in:
Authors: Jonathan Kliem Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 699d4e8 (Commits, GitHub, GitLab) Commit: 699d4e8f6bd2dcffa0c751cb9b1fa01923f9232c
Dependencies: #29903 Stopgaps:

Status badges

Description

We add a method that tests basic properties, when the TestSuite is run.

Change History (16)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/29905
  • Commit set to 6eaf649147590f3a72ea04841470cb117533cca4
  • Status changed from new to needs_review

New commits:

​0bdbe49test suites for the library
​5c7e562fix double description of hypercube
​206dbb7merged in public/29904
​c6ea1ecadded long time flags
​5d79b2bsome more testsuites
​6eaf649test some basic properties

comment:2 Changed 2 years ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:3 Changed 2 years ago by gh-kliem

Thank you.

comment:4 Changed 2 years ago by mkoeppe

  • Cc dimpase mjo added
  • Status changed from positive_review to needs_work

In light of the discussion on sage-devel (​https://groups.google.com/d/msg/sage-devel/c4UbKSdt3Aw/UQAo1iYoAAAJ), this needs more work

comment:5 Changed 2 years ago by gh-kliem

Why? What's wrong with this ticket?

Note that this ticket really has only one commit. If there is something wrong with #29903 or #29904 it should be fixed there. Not that I would know what the problem with those tickets would be.

One thing I could do is to use this ticket to improve the doctest for the hypercubes. Is that what you mean? Somethings as 

sage: P = polytopes.hypercube(intervals=random_intervals, backend='field')
sage: P._test_basic_properties()

comment:6 Changed 2 years ago by mkoeppe

The set_random_seed added in the doctest.

comment:7 Changed 2 years ago by gh-kliem

If that is the problem, then this ticket here is still green and #29903 should be "needs work". To avoid confusion I can of course quickly push one commit to all three tickets.

So we don't put set_random_seed anymore because we will go through with #29935 soon?

comment:8 Changed 2 years ago by mkoeppe

Please feel free to move the "needs work" to the correct ticket. Thanks

comment:9 Changed 2 years ago by mkoeppe

Yes, if you are concerned about the random seeds, this needs a proper solution, not one that is localized to one doctest.

comment:10 follow-up: Changed 2 years ago by mkoeppe

By the way, if you want to fuzz this particular doctest, why not just execute it in a loop so it picks up a new set of deterministic pseudo random numbers in each iteration? That should be good enough.

comment:11 Changed 2 years ago by git

  • Commit changed from 6eaf649147590f3a72ea04841470cb117533cca4 to 699d4e8f6bd2dcffa0c751cb9b1fa01923f9232c

Branch pushed to git repo; I updated commit sha1. New commits:

​699d4e8remove set_random seed

comment:12 in reply to: ↑ 10 Changed 2 years ago by gh-kliem

Replying to mkoeppe:

By the way, if you want to fuzz this particular doctest, why not just execute it in a loop so it picks up a new set of deterministic pseudo random numbers in each iteration? That should be good enough.

I already added a new doctest for #29904 that manually tests the only interesting case besides the static case that is always being tested.

comment:13 Changed 2 years ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:14 Changed 2 years ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:15 Changed 2 years ago by gh-kliem

Thank you.

comment:16 Changed 2 years ago by vbraun

  • Branch changed from public/29905 to 699d4e8f6bd2dcffa0c751cb9b1fa01923f9232c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.