Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass number of variables to var #22813

Closed
mforets mannequin opened this issue Apr 14, 2017 · 38 comments
Closed

Pass number of variables to var #22813

mforets mannequin opened this issue Apr 14, 2017 · 38 comments

Comments

@mforets
Copy link
Mannequin

mforets mannequin commented Apr 14, 2017

Extend the functionality of var by allowing to specify the number of variables:

sage: var('x', 4)
(x0, x1, x2, x3)

This ticket is a follow-up of #22809. It is related to #11576.

CC: @nbruin @kcrisman @tscrim

Component: symbolics

Keywords: symbolic, vector

Reviewer: Travis Scrimshaw

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

@mforets mforets mannequin added this to the sage-8.0 milestone Apr 14, 2017
@mforets mforets mannequin added c: symbolics labels Apr 14, 2017
@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 14, 2017

comment:1

Replying to nbruin : var(*args) already means the same thing as for a in args: var(a), so the signature is not available for different interpretation.

if we parse the arg by the length we get the desired behavior:

    if len(args)==1:
        name = args[0]
    elif len(args)==2:
        name = args[0]
        num_vars = args[1]
        if num_vars > 1:
            name = [name + str(i) for i in range(num_vars)]
    else:
        name = args

does it have some other undesired effects?

with the current set of tests in var.pyx, it seems to pass.

@nbruin
Copy link
Contributor

nbruin commented Apr 14, 2017

comment:2

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.

@tscrim
Copy link
Collaborator

tscrim commented Apr 14, 2017

comment:3

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.

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 15, 2017

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 15, 2017

comment:5

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:

b41d282add optional number of vars
34441b7add example to ticket's use case

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 15, 2017

Commit: 34441b7

@kcrisman
Copy link
Member

comment:6

There is, of course, the danger that this could overwrite someone's already-defined 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.

@tscrim
Copy link
Collaborator

tscrim commented Apr 15, 2017

comment:7

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.

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 15, 2017

comment:8

fair enough about naming conventions or handy constructions. as far as i'm concerned, this ticket is ready for peer-review!

@mforets mforets mannequin added the s: needs review label Apr 15, 2017
@nbruin
Copy link
Contributor

nbruin commented Apr 15, 2017

comment:9

Replying to @kcrisman:

There is, of course, the danger that this could overwrite someone's already-defined variables

I think that's one of the reasons why this doesn't belong in the top-level 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.

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 17, 2017

comment:10

Replying to @nbruin:

Replying to @kcrisman:

There is, of course, the danger that this could overwrite someone's already-defined variables

I think that's one of the reasons why this doesn't belong in the top-level 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.

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?

@tscrim
Copy link
Collaborator

tscrim commented Apr 17, 2017

comment:11

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2017

Changed commit from 34441b7 to 3d52b59

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2017

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

3d52b59prevent injecting components of x, return tuple instead

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 21, 2017

comment:13

Replying to @tscrim:

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.

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)
<ipython-input-21-e8a5da2185eb> 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 (?).

@jdemeyer
Copy link

comment:14

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 special-case num_vars == 0 and num_vars == 1? There should also be a check for negative numbers like var(x, -23).

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 21, 2017

comment:15

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 special-case num_vars == 0 and num_vars == 1? There should also be a check for negative numbers like var(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

@nbruin
Copy link
Contributor

nbruin commented Apr 21, 2017

comment:16

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2017

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

5212301use operator, fix injection

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2017

Changed commit from 3d52b59 to 5212301

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 21, 2017

comment:18

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

@nbruin
Copy link
Contributor

nbruin commented Apr 21, 2017

comment:19

Style in python prefers imports on top (see PEP-8). 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.

@nbruin
Copy link
Contributor

nbruin commented Apr 21, 2017

comment:20

I'm -1 on this injection and the place where this change happens. First the place:

SR.var is the main routine, which works side-effect free. Any API changes should be done there (and I think SR.var('x',3) would not get too much in the way). The top-level 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.

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 23, 2017

comment:21

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.

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 30, 2017

comment:22

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.

Thanks. This is #22909.

@mforets
Copy link
Mannequin Author

mforets mannequin commented May 25, 2017

comment:23

Is this is invalid/wontfix? Who does that? Thanks.

@mforets mforets mannequin added s: needs info and removed s: needs review labels May 25, 2017
@tscrim
Copy link
Collaborator

tscrim commented May 25, 2017

comment:24

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.

@mforets
Copy link
Mannequin Author

mforets mannequin commented May 27, 2017

comment:25

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 sage-devel, sage-support and ask.sage (plus those in a couple of old tickets). I don't plan to do this, at least not at the moment.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Mar 13, 2021

comment:26

This is fixed by #22909 (albeit requesting using the n= keyword...).

==> marking as duplicate and requesting review in order to solve...

@EmmanuelCharpentier EmmanuelCharpentier mannequin removed this from the sage-8.0 milestone Mar 13, 2021
@kcrisman
Copy link
Member

comment:27

As you can see from the discussion, at least Travis thought this wasn't 100% resolved, since this ticket is now about injection into the global namespace of the SR.var from #22909.

Of course, we could also close this as undesirable or something not to work on now but rather if it comes up again. Marcelo and Nils seemed to agree with me that maybe that wasn't bad. See if Travis responds (he's usually pretty aware of relevant ticket changes he's cc:ed on) otherwise I this we can indeed close this, though as wontfix rather than a dup.

@kcrisman kcrisman added this to the sage-9.4 milestone Mar 13, 2021
@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Mar 13, 2021

comment:28

Replying to @kcrisman:

As you can see from the discussion, at least Travis thought this wasn't 100% resolved, since this ticket is now about injection into the global namespace of the SR.var from #22909.

The current behavior seems OK :

sage: var("z", n=2)
(z0, z1)
sage: z0
z0
sage: z
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-10-3a710d2a84f8> in <module>
----> 1 z

NameError: name 'z' is not defined

Users can always do z=var("z", n=2 if needed, but might prefer Z=var("z", n=2 or {{{, if only for pedagogical reasons.

Furthermore, we have so far just a trick to define a bunch of symbolic variables easily, but no mathematical properties associated with this bunch.

We could want to do something like z=vector(var("z", n=2)), but is this really what we mean ? It seems to me that this restricts the possible (or at least the easy) use cases of these indexed variables, since it implies that those symbolic variables have common properties (e. g. their possible values have the same parent...).

Such an addition would be extremely tempting, however, if we could create "symbolic variables of a fixed type", which do not exist currently in Sage (e. g. there is no way to create a polynomial with, say, rational coefficients designated by names rather than values...).

Of course, we could also close this as undesirable or something not to work on now but rather if it comes up again. Marcelo and Nils seemed to agree with me that maybe that wasn't bad. See if Travis responds (he's usually pretty aware of relevant ticket changes he's cc:ed on) otherwise I this we can indeed close this, though as wontfix rather than a dup.

As far as I know, this the same closure ("duplicate/invalid/wontfix")...

@nbruin
Copy link
Contributor

nbruin commented Mar 14, 2021

comment:29

I wouldn't say the current behaviour is "OK", but it's a fact that we already have that:

sage: var("z",n=2)
sage: z0,z1

so it's already injecting the (individual) variables. As I explained above, I expect that this is one of 3 outcomes:

  • var("z",n=2) should bind the names of the symbols it creates, or
  • var("z",n=2) should bind z to a tuple so that z[0] and z[1] are the desired symbols, or
  • var("z",n=2) shouldn't bind anything because we don't know what to bind: the names z0,z1 aren't explicitly mentioned, so those should be out-of-bounds, and z isn't really created as a symbol itself.
    Personally I think: var(...) shouldn't support n=... at all because it's unclear what it should do. People should be steered to SR.var which forces people to perform the desired binding behaviour. That ship has sailed, though. And if people get confused by var then we can just say: use SR.var instead.

so +1 on closing as duplicate/wontfix

@tscrim
Copy link
Collaborator

tscrim commented Mar 15, 2021

comment:30

I agree with Nils that the behavior needs to be better specified, which means there needs to be better documentation. So I am okay with closing this as duplicate, but we might want to consider update the documentation of var() to make it clear what the behavior is. We can argue later about what the behavior should be.

Which, getting towards that later argument, I see Nils point about binding. I typically want the tuple and utilize that because I will later on iterate over the variables. So I might lean towards that behavior rather than injecting the individual variables. However, I can see a lot of cases where I would instead just use the individual variables, which I feel would be more in line with a less programming-friendly user. There is some advantage to the current behavior that you can have the best of both worlds with z = var('z', n=5).

@kcrisman
Copy link
Member

comment:31

Okay. Then let's close this one, and Travis can you open a new ticket that describes exactly what you are thinking of? I don't have a horse in this race other than making sure everything is meta-documented on Trac :-)

@tscrim
Copy link
Collaborator

tscrim commented Mar 22, 2021

comment:32

We can just refer to this ticket for now until we have a more concrete proposal. I am also stretched a little too thin right now to put much effort into this.

@tscrim
Copy link
Collaborator

tscrim commented Mar 22, 2021

Changed commit from 5212301 to none

@tscrim
Copy link
Collaborator

tscrim commented Mar 22, 2021

Changed branch from u/mforets/pass_number_of_variables_to_var to none

@tscrim
Copy link
Collaborator

tscrim commented Mar 22, 2021

Reviewer: Travis Scrimshaw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants