Opened 8 years ago

Closed 5 years ago

#12298 closed defect (fixed)

minor CallableSymbolicExpressionRing display bug.

Reported by: gbe Owned by: gbe
Priority: trivial Milestone: sage-6.5
Component: symbolics Keywords: beginner, sd35.5
Cc: kcrisman Merged in:
Authors: Geoffrey Ehrman, Kannappan Sampath, Sergey Bykov Reviewers: Kannappan Sampath, Karl-Dieter Crisman, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 6deecf1 (Commits) Commit: 6deecf1895185e4ae927ceb6d623e5bca66410b3
Dependencies: Stopgaps:

Description

When a CallableSymbolicExpressionRing? is created with only one variable, arguments is still plural and an extra comma is printed.

Correct, #variable > 1: 

sage: CallableSymbolicExpressionRing([var('q p')])
Callable function ring with arguments (q, p)

Incorrect: 

sage: CallableSymbolicExpressionRing([var('q')])
Callable function ring with arguments (q,)

The extra comma, I suspect, is because the repr returns something like "... arguments (%s)" % variable_tuple. This is what would be expected of a singular tuple.

Attachments (2)

trac_12298.patch​ (1.5 KB) - added by gbe 8 years ago.
trac_12298_doctest_add.patch​ (2.8 KB) - added by knsam 7 years ago.
Adds doctests and introduces minor coding style changes.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by gbe

  • Cc gbe removed

Changed 8 years ago by gbe

comment:2 Changed 8 years ago by gbe

  • Keywords sd35.5 added
  • Status changed from new to needs_review

comment:3 Changed 8 years ago by kcrisman

  • Cc kcrisman added

comment:4 Changed 8 years ago by kcrisman

  • Authors set to Geoffrey Ehrman
  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

This looks fine but needs doctests.

Changed 7 years ago by knsam

Adds doctests and introduces minor coding style changes.

comment:5 Changed 7 years ago by knsam

  • Status changed from needs_work to needs_review

comment:6 follow-up: Changed 7 years ago by kcrisman

Thanks. Given that you are making these changes, should we fix any other little changes like that and add this file to the reference manual? See e.g. ​here. Though perhaps that should be based off of #6495 rather than interfere with that very big patch...

comment:7 in reply to: ↑ 6 Changed 7 years ago by knsam

Replying to kcrisman: Hello!

Thanks. Given that you are making these changes, should we fix any other little changes like that and add this file to the reference manual? See e.g. ​here. Though perhaps that should be based off of #6495 rather than interfere with that very big patch...

We could sure do that. We'll have to find an appropriate place to add this. I'd be glad to hear your suggestion, but at the moment, I am inclined to add it just after sage/symbolic/function (equivalently, just before sage/symbolic/integration/integral).

comment:8 Changed 7 years ago by kcrisman

  • Authors changed from Geoffrey Ehrman to Geoffrey Ehrman, Kannappan Sampath
  • Reviewers changed from Karl-Dieter Crisman to Kannappan Sampath, Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

See patchbot result. 

sage -t  -force_lib devel/sage-12298/sage/modules/free_module_element.pyx
**********************************************************************
File "/scratch/sage-5.8.beta4/devel/sage-12298/sage/modules/free_module_element.pyx", line 3900:
    sage: w.base_ring()
Expected:
    Callable function ring with arguments (x,)
Got:
    Callable function ring with argument x
**********************************************************************

Should be easy to fix. Maybe you can add this to the reference manual at the place you suggest, too.

Otherwise this is fine, though if you're already doing it you may want to switch to Python 3 string formatting - up to you.

comment:9 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:10 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:11 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:12 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:13 Changed 5 years ago by captaintrunky

  • Branch set to u/captaintrunky/minor_callablesymbolicexpressionring_display_bug_

comment:14 Changed 5 years ago by captaintrunky

  • Authors changed from Geoffrey Ehrman, Kannappan Sampath to Geoffrey Ehrman, Kannappan Sampath, Sergey Bykov
  • Commit set to 6deecf1895185e4ae927ceb6d623e5bca66410b3
  • Status changed from needs_work to needs_review

New commits:

​6deecf1Fixes a misspelling at src/sage/categories/pushout.py and the docstring test with CallableSymbolicExpressionRing

comment:15 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-6.4 to sage-6.5
  • Reviewers changed from Kannappan Sampath, Karl-Dieter Crisman to Kannappan Sampath, Karl-Dieter Crisman, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:16 Changed 5 years ago by vbraun

  • Branch changed from u/captaintrunky/minor_callablesymbolicexpressionring_display_bug_ to 6deecf1895185e4ae927ceb6d623e5bca66410b3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.