Opened 23 months ago
Last modified 21 months ago
#22813 needs_info enhancement
Pass number of variables to var
Reported by:  mforets  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  symbolics  Keywords:  symbolic, vector 
Cc:  nbruin, kcrisman, tscrim  Merged in:  
Authors:  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/mforets/pass_number_of_variables_to_var (Commits)  Commit:  521230176b40e38fd1f638da4e4a206edcd03d75 
Dependencies:  Stopgaps: 
Change History (25)
comment:1 Changed 23 months ago by
comment:2 followup: ↓ 14 Changed 23 months ago by
Problem: var("x","y")
, as long as "y">1
(which is true at the moment). Problem in python: duck typing. The second argument could be "stringy" or "integery" or both. In the "both" case, there's no way to choose.
comment:3 Changed 23 months ago by
IIRC 'y' > 1
will fail in Python3. However, you could do isinstance(args[1], str)
to get around the ducktyping. It's not like any of this code needs to be super fast/lightweight.
comment:4 Changed 22 months ago by
 Branch set to u/mforets/pass_number_of_variables_to_var
comment:5 Changed 22 months ago by
 Commit set to 34441b77670a1f81e57c030a2fa246b95ecbb8ae
Added the numeric 2nd argument, following @tscrim's recommendation.
Let me suggest to add functionality for handling more than one variable. Note that the behavior below is a replica of SymPy?'s symbols constructor, check from sympy import symbols
.
It is possible to handle several variables if we pass a list of names as first argument:: sage: var(['x', 'y'], 2) ((x0, x1), (y0, y1))
and
The input variable name(s) is taken as initial subindex:: sage: var(['x1', 'y2'], 2) ((x1, x2), (y2, y3))
What are your thoughts?
New commits:
b41d282  add optional number of vars

34441b7  add example to ticket's use case

comment:6 followup: ↓ 9 Changed 22 months ago by
There is, of course, the danger that this could overwrite someone's alreadydefined variables  say, if
x3=end_of_computation_of_last_twin_prime_pair # Fields Medal in hand! var('x',5) # oops
though of course that can happen anyway, this is just more silent about it. Perhaps a check to see whether any of the variables about to be produced is already defined? Though we don't currently do that and the same danger exists... this is just more implicit, which isn't very Pythonic.
None of this should be construed as not supporting the goal of this ticket, I'm just raising questions.
comment:7 Changed 22 months ago by
I would expect var('y1', 2)
to give me y10, y11
, so having that be the initial value would be very surprising. So 1 on parsing out the number of the variable to be the start index.
comment:8 Changed 22 months ago by
 Status changed from new to needs_review
fair enough about naming conventions or handy constructions. as far as i'm concerned, this ticket is ready for peerreview!
comment:9 in reply to: ↑ 6 ; followup: ↓ 10 Changed 22 months ago by
Replying to kcrisman:
There is, of course, the danger that this could overwrite someone's alreadydefined variables
I think that's one of the reasons why this doesn't belong in the toplevel var
. I could see why people would want to have a shorthand for
lambda x,n: tuple(SR.symbol("%s%s"%(x,i)) for in range(n))
or perhaps even
lambda x,n,m: tuple(tuple(SR.symbol("%s%s_%s"%(x,i,j)) for j in range(m)) for i in range(n))
but you'd probably want to bind the result to a single symbol, say X
, so that you can spell variables programmatically via X[i]
or X[i][j]
.
This is also the approach taken in
sage: R.<X>=InfinitePolynomialRing(QQ)
where you need to use X[0],X[1]
etc.
In other words, generating indexed symbolic variables is probably not suitable for injection at all.
comment:10 in reply to: ↑ 9 Changed 22 months ago by
Replying to nbruin:
Replying to kcrisman:
There is, of course, the danger that this could overwrite someone's alreadydefined variables
I think that's one of the reasons why this doesn't belong in the toplevel
var
. I could see why people would want to have a shorthand forlambda x,n: tuple(SR.symbol("%s%s"%(x,i)) for in range(n))or perhaps even
lambda x,n,m: tuple(tuple(SR.symbol("%s%s_%s"%(x,i,j)) for j in range(m)) for i in range(n))but you'd probably want to bind the result to a single symbol, say
X
, so that you can spell variables programmatically viaX[i]
orX[i][j]
. This is also the approach taken insage: R.<X>=InfinitePolynomialRing(QQ)where you need to use
X[0],X[1]
etc.In other words, generating indexed symbolic variables is probably not suitable for injection at all.
OK, it makes sense. Moreover it also answers my previous question if there is a technical reason why var
wasn't defined to work with vectors/matrices already in Sage (.. I was comparing to Matlab's sym('x', m, n)
command, but that one only returns the object, it does no injection).
I guess this is wontfix?
comment:11 followup: ↓ 13 Changed 22 months ago by
Perhaps what we want is the output of var('x', 3)
to inject the tuple x
of variables (x0, x1, x2)
. This would be in line with polynomial rings and what we do with other calls to var
.
comment:12 Changed 22 months ago by
 Commit changed from 34441b77670a1f81e57c030a2fa246b95ecbb8ae to 3d52b597053ce271b8956b84112391694c511d33
Branch pushed to git repo; I updated commit sha1. New commits:
3d52b59  prevent injecting components of x, return tuple instead

comment:13 in reply to: ↑ 11 Changed 22 months ago by
Replying to tscrim:
Perhaps what we want is the output of
var('x', 3)
to inject the tuplex
of variables(x0, x1, x2)
. This would be in line with polynomial rings and what we do with other calls tovar
.
Good, i've uploaded an attempt to implement it. Feedback welcome. The behavior is now:
sage: x = var('x', 3) sage: x (x0, x1, x2) sage: x1  NameError Traceback (most recent call last) <ipythoninput21e8a5da2185eb> in <module>() > 1 x1 NameError: name 'x1' is not defined
BTW this is related to #11576, a summary follows. There are pointers to use cases where this notation is useful. There are also many technical hints explaining that one would prefer to exploit Ginac's indexed expressions. There are patches, the last one with a doctests failure reported in sage 5.10.
As far as i can see, a random user would still need, currently, the convoluted a = var(','.join('a%s'%i for i in range(4))); a
, hence comparable to a = var('a', 4)
in terms of performance (?).
comment:14 in reply to: ↑ 2 ; followup: ↓ 15 Changed 22 months ago by
Instead of checking for a string and assuming it's a number otherwise, I'd rather do it the other way around: check for a number with operator.index()
. If that fails, assume it's a string.
I also dislike the if num_vars > 1
: why specialcase num_vars == 0
and num_vars == 1
? There should also be a check for negative numbers like var(x, 23)
.
comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 22 months ago by
Hi,
Replying to jdemeyer:
Instead of checking for a string and assuming it's a number otherwise, I'd rather do it the other way around: check for a number with
operator.index()
. If that fails, assume it's a string.
do you mean if hasattr(args[1], '__index__')
?
I also dislike the
if num_vars > 1
: why specialcasenum_vars == 0
andnum_vars == 1
? There should also be a check for negative numbers likevar(x, 23)
.
this was intended for the case var('x', 1)
, which should return i suppose x0
. in this situation, passing a list is wrong because the command SR.var(['x0'])
is not ok (gives TypeError: unhashable type: 'list'
). so i didn't know if there is a way to bypass this issue without making the 2 cases.
with those changes, the if
becomes:
got_number = False if len(args)==1: name = args[0] elif len(args)==2: if hasattr(args[1], '__index__'): got_number = True num_vars = args[1] if num_vars == 1: name = (args[0] + '0') elif num_vars > 1: name = [args[0] + str(i) for i in range(num_vars)] else: raise ValueError("The number number of variables should be at least 1") else: name = args else: name = args
comment:16 in reply to: ↑ 15 ; followup: ↓ 18 Changed 22 months ago by
Replying to mforets:
do you mean
if hasattr(args[1], '__index__')
?
I'm pretty sure he rather meant
try: num_vars = operator.index(args[1]) ... except TypeError: name = args
comment:17 Changed 22 months ago by
 Commit changed from 3d52b597053ce271b8956b84112391694c511d33 to 521230176b40e38fd1f638da4e4a206edcd03d75
Branch pushed to git repo; I updated commit sha1. New commits:
5212301  use operator, fix injection

comment:18 in reply to: ↑ 16 Changed 22 months ago by
Replying to nbruin:
Replying to mforets:
do you mean
if hasattr(args[1], '__index__')
?I'm pretty sure he rather meant
try: num_vars = operator.index(args[1]) ... except TypeError: name = args
ok. for the import operator
statement, in other files this is done at the script level (here, var.pyx
), while at others it is at the function level (here, def var
). assuming that the condition is that to be on top it should be used by more than 1 function, i've put it inside def var
.
updated version with:
 case of 1 variable :
sage: polygens(QQ, 'x', 1) # returns a tuple (x,) sage: var('x', 1) # returns a tuple, too (x0,) sage: var('x') # usual, does not return a tuple x
 check tuple is injected properly :
sage: var('z', 2) (z0, z1) sage: z (z0, z1)
critics/corrections welcome
comment:19 Changed 22 months ago by
Style in python prefers imports on top (see PEP8). There are reasons to deviate, and reasons not to. The answer at stackoverflow seems pretty comprehensive and sensible.
Most imports in functions in sage are to avoid expensive imports on startup and to avoid some circular import problems with from ... import ...
statements. I don't think that applies here, so the import should go on top.
comment:20 followups: ↓ 21 ↓ 22 Changed 22 months ago by
I'm 1 on this injection and the place where this change happens. First the place:
SR.var
is the main routine, which works sideeffect free. Any API changes should be done there (and I think SR.var('x',3)
would not get too much in the way). The toplevel var
only differs from SR.var
in order to support binding injection.
Next the kind of injection.
With var('z',2)
the return value is (z0,z1)
, while the binding is to the name indicated by the string z
. I think in most cases people who use this would want to do z=SR.var('z',2)
, which expresses the result perfectly and also reflects how a significant number of examples in our documentation use var
(as in x=var('x')
). However, it is not obvious to me that people would automatically expect var('z',2)
to have that effect. I expect that half of them would expect z0,z1
to be bound, because that is what var('z0,z1')
does and it has the same return value.
That's confusing.
Hence: generating variable sequences is fine, but the "right" injection behaviour here is even less clear than in a simple var
. Starting afresh, I would not expect var
to have its injection behaviour at all. Some magic command like %var x
or a preprocessor construct is a much more natural syntax vehicle for it. In this case, I don't see one obvious way of extending var
's injection behaviour in a natural way, so I think this shouldn't be done at all.
First put it in SR.var and see what usage arises. Perhaps practical experience shows how (if at all) injection should work.
comment:21 in reply to: ↑ 20 Changed 22 months ago by
Replying to nbruin:
First put it in SR.var and see what usage arises. Perhaps practical experience shows how (if at all) injection should work.
ok! i'm ready to write the code for SR.var (new ticket). do you mind creating it? (i don't know if doing that right now is ok or on the contrary it is too soon to take that decision..)
for the full discussion, true, now i see that you want to avoid var('z', 2)
and var('z0, z1')
behaving differently.
comment:22 in reply to: ↑ 20 Changed 22 months ago by
comment:23 Changed 21 months ago by
 Cc tscrim added
 Status changed from needs_review to needs_info
Is this is invalid/wontfix? Who does that? Thanks.
comment:24 followup: ↓ 25 Changed 21 months ago by
You would set the milestone to invald/wontfix, but I am not sure that is what we want to do at this point. #22909 is a different issue and having this would be useful. I think there is still some discussion to be had as I feel the injection issue is no worse than the current situation. I think we should come to a bit more of a consensus before rendering this ticket invalid.
comment:25 in reply to: ↑ 24 Changed 21 months ago by
Replying to tscrim:
You would set the milestone to invald/wontfix, but I am not sure that is what we want to do at this point. #22909 is a different issue and having this would be useful. I think there is still some discussion to be had as I feel the injection issue is no worse than the current situation. I think we should come to a bit more of a consensus before rendering this ticket invalid.
That's fine. If a new discussion takes place, it would be nice if it kicks off with a summary of the several threads on this topic that exist in sagedevel, sagesupport and ask.sage (plus those in a couple of old tickets). I don't plan to do this, at least not at the moment.
if we parse the arg by the length we get the desired behavior:
does it have some other undesired effects?
with the current set of tests in
var.pyx
, it seems to pass.