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

[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.

Component: misc

Author: Reimundo Heluani

Branch/Commit: 725d4b5

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/29797

[heluani]

Branch: u/heluani/sage_misc_defaults_variable_names_return_tuple_in_all_cases

[heluani]

Commit: 1570469

[heluani]

Author: Reimundo Heluani

[tscrim]

comment:3

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.

[tscrim]

Reviewer: Travis Scrimshaw

[DaveWitteMorris]

comment:4

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.

[tscrim]

comment:5

Replying to @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.

[sagetrac-git]

Changed commit from 1570469 to 725d4b5

[sagetrac-git]

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

725d4b5

Changes requested by comments #3 and #4

[heluani]

comment:7

Replying to @tscrim:

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

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

[nbruin]

comment:8

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.

[DaveWitteMorris]

comment:9

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.)

[vbraun]

Changed branch from u/heluani/sage_misc_defaults_variable_names_return_tuple_in_all_cases to 725d4b5