#29797 closed defect (fixed)

sage.misc.defaults.variable_names return tuple in all cases

Reported by: heluani Owned by:
Priority: major Milestone: sage-9.2
Component: misc Keywords:
Cc: Merged in:
Authors: Reimundo Heluani Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 725d4b5 (Commits, GitHub, GitLab) Commit: 725d4b5978a92f4ecf3b46911b16d0443c1af616
Dependencies: Stopgaps:

Status badges

Description (last modified by heluani)

sage.misc.defaults.variable_names and latex_variable_names return tuple if the number of variable names requested is bigger than 1 or 0 and a list if it's 1. This ticket fixes that.

Change History (10)

comment:1 Changed 13 months ago by heluani

  • Branch set to u/heluani/sage_misc_defaults_variable_names_return_tuple_in_all_cases

comment:2 Changed 13 months ago by heluani

  • Authors set to Reimundo Heluani
  • Commit set to 15704692e56d905577f721a4e0f53898f24e7107
  • Component changed from PLEASE CHANGE to misc
  • Description modified (diff)
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

comment:3 follow-up: Changed 13 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

For safety:

 def latex_variable_names(n, name=None):
-    """
+    r"""
     Converts a root string into a tuple of variable names by adding 
     numbers in sequence.

Also, minor nitpick with the doc formatting:

-    - ``n`` a non-negative Integer. The number of variable names to 
-       output.
-    - ``names`` a string (Default: ``None``); The root of the variable
-      name. 
+    - ``n`` a non-negative Integer; the number of variable names to 
+       output
+    - ``names`` a string (default: ``None``); the root of the variable
+      name.

Once changed, you can set a positive review on my behalf.

comment:4 follow-up: Changed 13 months ago by gh-DaveWitteMorris

Thanks for making the fix, but I think there are space characters at the ends of some lines (and a line with only spaces). Please get rid of those spaces. Also, since you are editing the file, please change tuple([ ... ]) to tuple( ... ) (twice, near the end of both functions), even though this is not your fault.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 13 months ago by tscrim

Replying to gh-DaveWitteMorris:

Also, since you are editing the file, please change tuple([ ... ]) to tuple( ... ) (twice, near the end of both functions), even though this is not your fault.

I am -1 on this change because it is actually slower (surprisingly) to not have the inner list comprehension.

comment:6 Changed 13 months ago by git

  • Commit changed from 15704692e56d905577f721a4e0f53898f24e7107 to 725d4b5978a92f4ecf3b46911b16d0443c1af616

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

725d4b5Changes requested by comments #3 and #4

comment:7 in reply to: ↑ 3 Changed 13 months ago by heluani

  • Status changed from needs_review to positive_review

Replying to tscrim:

Once changed, you can set a positive review on my behalf.

Was I suppose to make this change to 'positive review'?

comment:8 in reply to: ↑ 5 Changed 13 months ago by nbruin

Replying to tscrim:

I am -1 on this change because it is actually slower (surprisingly) to not have the inner list comprehension.

Indeed! There's no "tuple comprehension" in python; just the possibility to make a tuple from an iterator. You can save the "tuple" by writing

(*[...],)

but as far as I can see this has about the same performance as tuple([...]) has. I guess the tuple constructor misses some of the optimizations of list comprehensions to build a list with beforehand-unknown-length. Passing a list into tuple gives the length beforehand, so it's easier to construct the tuple efficiently.

comment:9 Changed 13 months ago by gh-DaveWitteMorris

OK, I stand corrected. I made the suggestion because it was a reviewer comment on one of the patches I wrote, and made sense to me. (I did some quick tests on this case and I am usually not seeing any significant difference in performance, but tuple([...]) does come out faster sometimes, and I can believe that it is a better general practice, even though it is counterintuitive.)

comment:10 Changed 12 months ago by vbraun

  • Branch changed from u/heluani/sage_misc_defaults_variable_names_return_tuple_in_all_cases to 725d4b5978a92f4ecf3b46911b16d0443c1af616
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.