Opened 5 years ago

Closed 5 years ago

#16228 closed defect (fixed)

make tachyon respect standard verbosity settings

Reported by: niles Owned by:
Priority: major Milestone: sage-6.4
Component: graphics Keywords: tachyon, verbose, beginner
Cc: Merged in:
Authors: Paul Graham Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 0d85789 (Commits) Commit: 0d857894ac1af964695d3e082d6fdc3bc46bfc67
Dependencies: Stopgaps:

Description

Tachyon.save and Tachyon.show have their own arguments for verbose output and do not pay attention to the verbosity levels set by sage.misc.misc.set_verbose. This makes for inconsistent and confusing interface.

Instead of checking the value of an optional argument, these methods should check sage's verbosity setting. This is done implicitly when messages are printed with sage.misc.misc.verbose. An explanation of this change should be added to the documentation.

Change History (15)

comment:1 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:2 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:3 Changed 5 years ago by paulgraham5

Since Tachyon.show calls Tachyon.save should Tachyon.save check the verbosity level and pass it to tachyon_rt in sage.interfaces.tachyon? or should all references to the verbosity level be removed from Tachyon.show and Tachyon.save and just check the verbosity level in tachyon_rt?

comment:4 Changed 5 years ago by dimpase

I'd not completely agree with the task as set out in the ticket description. Namely, it should be OK to use optional arguments to override the Sage's verbosity setting. This way, Tachyon.show/save should check their verbosity arguments, and if none are give, pass to tachyon_rt Sage's verbosity. Otherwise, they should pass the value of the optional argument given.

comment:5 Changed 5 years ago by paulgraham5

  • Branch set to u/paulgraham5/make_tachyon_respect_standard_verbosity_settings

comment:6 Changed 5 years ago by paulgraham5

  • Authors set to Paul Graham
  • Commit set to 8246af4a1ae7ee9fa6656ba0f7fa8e39bc7daa29
  • Status changed from new to needs_review

New commits:

8246af4Made tachyon use the standard verbosity setting as default.

comment:7 follow-up: Changed 5 years ago by dimpase

Can you provide doctests illustrating the effects of setting sage.misc.misc.set_verbose ?

(don't forget to set sage.misc.misc.set_verbose back to its original value in such a doctest)

comment:8 Changed 5 years ago by paulgraham5

Last edited 5 years ago by paulgraham5 (previous) (diff)

comment:9 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by paulgraham5

Replying to dimpase:

Can you provide doctests illustrating the effects of setting sage.misc.misc.set_verbose ?

(don't forget to set sage.misc.misc.set_verbose back to its original value in such a doctest)

Okay. How do you normally go about creating doctests? I haven’t done that before. From reading http://www.sagemath.org/doc/developer/doctesting.html#beyond-the-sage-library , i gather that, like in the examples section of a function definition eg:

        r"""
        Creates a PNG file of the scene.

        EXAMPLES::

            sage: q = Tachyon()
            sage: q.light((-1,-1,10), 1,(1,1,1))
            sage: q.texture('s')
            sage: q.sphere((0,0,0),1,'s')
            sage: q.show(verbose=2)
        """

I put these further examples in this section, then run $ ./sage -t src/sage/plot/plot3d/tachyon.py ? Or Are you referring to another method? :)

comment:10 in reply to: ↑ 9 Changed 5 years ago by niles

Replying to paulgraham5:

I put these further examples in this section, then run $ ./sage -t src/sage/plot/plot3d/tachyon.py ?

Right :)

Also, build the documentation and check that it looks good in the reference manual.

Last edited 5 years ago by niles (previous) (diff)

comment:11 Changed 5 years ago by git

  • Commit changed from 8246af4a1ae7ee9fa6656ba0f7fa8e39bc7daa29 to 50f9a14761c6b74b7dca95f3048b0d108f2163b3

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

50f9a14Introduced doctests to illustrate effects of changing global verbosity.

comment:12 Changed 5 years ago by paulgraham5

The reference manual looks okay, and the tests go through okay on my end. The doc-tests are a new concept to me, so hopefully they aren’t too bad :) .

comment:13 Changed 5 years ago by chapoton

  • Branch changed from u/paulgraham5/make_tachyon_respect_standard_verbosity_settings to public/ticket/16228
  • Commit changed from 50f9a14761c6b74b7dca95f3048b0d108f2163b3 to 0d857894ac1af964695d3e082d6fdc3bc46bfc67

I rebased on 6.4.beta2


New commits:

0d85789Merge with 6.4.beta2

comment:14 Changed 5 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good to me.

comment:15 Changed 5 years ago by vbraun

  • Branch changed from public/ticket/16228 to 0d857894ac1af964695d3e082d6fdc3bc46bfc67
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.