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

Indexed SR variables #22909

Closed
mforets mannequin opened this issue Apr 30, 2017 · 29 comments
Closed

Indexed SR variables #22909

mforets mannequin opened this issue Apr 30, 2017 · 29 comments

Comments

@mforets
Copy link
Mannequin

mforets mannequin commented Apr 30, 2017

Provide a keyword argument to construct a tuple of symbolic variables:

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

CC: @nbruin @kcrisman @rwst

Component: symbolics

Keywords: symbolic ring

Author: Marcelo Forets

Branch/Commit: 29c9c2d

Reviewer: Travis Scrimshaw

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

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

mforets mannequin commented Apr 30, 2017

Commit: 3093d25

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 30, 2017

comment:1

Related to: #22813, #22809, and #11576.


New commits:

7b8eb5fadd index keyword in SR.var
3093d25add multiple indexing error

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 30, 2017

Author: Marcelo Forets

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 30, 2017

Branch: u/mforets/22909

@mforets mforets mannequin added the s: needs review label Apr 30, 2017
@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2017

comment:2

This change is backwards incompatible with var('x','y'), where the second argument is treated as the latex name. We should also handle the latex name too.

The argument name of index is misleading. Perhaps n, index_set, indicies, or number?

Little thing, but the error messages should start with a lowercase letter (at least all new messages).

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 30, 2017

comment:3

Replying to @tscrim:

as usual, thanks for your prompt feedback!!

This change is backwards incompatible with var('x','y'), where the second argument is treated as the latex name. We should also handle the latex name too.

yes. with the current commit one has:

sage: SR.var('x', 'y')
...
AttributeError: 'str' object has no attribute 'is_integer'

instead of:

sage: SR.var('x', latex_name='y')
x
sage: latex(x)
{y}

should i do this with some type checking to handle (integer/string), or just ordering the arguments in a different way and expect user to always type index=...?

IMO, it is better if SR.var('x', 4) is understood by the program.

The argument name of index is misleading. Perhaps n, index_set, indicies, or number?

i used index because one speaks of "indexed variables". for those you suggest let me upvote (in order of preference): n and indices. in addition to those, let me add as options: len, length or size. what do others think?

Little thing, but the error messages should start with a lowercase letter (at least all new messages).

ok, and full stop yes or no?

the question is if an exception should be a complete sentence or not.

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2017

comment:4

Replying to @mforets:

Replying to @tscrim:
This change is backwards incompatible with var('x','y'), where the second argument is treated as the latex name. We should also handle the latex name too.

yes. with the current commit one has:

sage: SR.var('x', 'y')
...
AttributeError: 'str' object has no attribute 'is_integer'

instead of:

sage: SR.var('x', latex_name='y')
x
sage: latex(x)
{y}

should i do this with some type checking to handle (integer/string), or just ordering the arguments in a different way and expect user to always type index=...?

As you surmised, you can do one of two things. The first is to put the n (index) as a later argument (I feel that domain is less used except as a keyword argument, so I'm more willing to let that be backwards incompatible). The second, and more preferable, is to check the input type and redirect as necessary. (This is one of the biggest shortcomings of weakly type languages.)

IMO, it is better if SR.var('x', 4) is understood by the program.

I agree. It just means some more complicated code to parse the inputs, but it has better useability.

The argument name of index is misleading. Perhaps n, index_set, indicies, or number?

i used index because one speaks of "indexed variables". for those you suggest let me upvote (in order of preference): n and indices. in addition to those, let me add as options: len, length or size. what do others think?

I think n is generic but clear enough for something like this. I don't have too much of a preference (but not index).

Little thing, but the error messages should start with a lowercase letter (at least all new messages).

ok, and full stop yes or no?

the question is if an exception should be a complete sentence or not.

They are generally not, at least by Python's standard. So no full stop nor uppercase letter.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 2, 2017

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

8ad4a06fix keyword name and exception formatting
751d7dfcheck input types and redirect as necessary

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 2, 2017

Changed commit from 3093d25 to 751d7df

@tscrim
Copy link
Collaborator

tscrim commented May 3, 2017

comment:6

I'm cc-ing Nils, Karl-Dieter, and Ralf to see if they have any comments before setting this to a positive review.

@kcrisman
Copy link
Member

kcrisman commented May 3, 2017

comment:7

I'm unsure if I have a preference, but keep in mind the injection issue discussed elsewhere - I don't know whether these should be injected automatically into the global namespace or not. Not that I have a preference on that either :-) but just be sure to see, I didn't follow that discussion carefully.

@tscrim
Copy link
Collaborator

tscrim commented May 3, 2017

comment:8

FYI, this does not inject any variables. It does add it to SR.symbols however, but I think that is less of an issue.

@mforets
Copy link
Mannequin Author

mforets mannequin commented May 3, 2017

comment:9

also as you can see there is no "INPUT" block in this function. if you want that can be added, together with a list of possible domains that one could use.

@nbruin
Copy link
Contributor

nbruin commented May 3, 2017

comment:10

Don't use a bare except. Put as narrow an exception class there as possible. Unintentionally catching exceptions makes debugging a lot harder.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2017

Changed commit from 751d7df to 3d508e4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2017

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

3d508e4specify exception type

@videlec
Copy link
Contributor

videlec commented May 4, 2017

comment:12

See also the related #18390

@mforets
Copy link
Mannequin Author

mforets mannequin commented May 7, 2017

comment:13

(i'd like to add a proper docstring to this function) reference page of SR

@mforets mforets mannequin added s: needs work and removed s: needs review labels May 7, 2017
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2017

Changed commit from 3d508e4 to 5676d25

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2017

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

5676d25update docstring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 22, 2017

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

ac9d797Merge branch 'develop' into t/mforets/pass_number_of_variables_to_SRvar
f34ac33SR.var docstring update

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 22, 2017

Changed commit from 5676d25 to f34ac33

@mforets mforets mannequin removed the s: needs work label May 22, 2017
@mforets mforets mannequin added the s: needs review label May 22, 2017
@tscrim
Copy link
Collaborator

tscrim commented May 22, 2017

comment:17

Last little thing from me: this could be done

-'{{{0}}}'.format(latex_name) + '_{{{0}}}'.format(str(i))
+'{{{}}}_{{{}}}'.format(latex_name, str(i))

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 24, 2017

Changed commit from f34ac33 to 29c9c2d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 24, 2017

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

29c9c2dsimplify formatted lated name line

@mforets
Copy link
Mannequin Author

mforets mannequin commented May 24, 2017

comment:19

Replying to @tscrim:

Last little thing from me: this could be done

-'{{{0}}}'.format(latex_name) + '_{{{0}}}'.format(str(i))
+'{{{}}}_{{{}}}'.format(latex_name, str(i))

done.

@tscrim
Copy link
Collaborator

tscrim commented May 24, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented May 24, 2017

comment:20

If there are any objections, then set this back from positive review.

@vbraun
Copy link
Member

vbraun commented May 26, 2017

Changed branch from u/mforets/22909 to 29c9c2d

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