Opened 2 years ago

Closed 2 years ago

#22909 closed enhancement (fixed)

Indexed SR variables

Reported by: mforets Owned by:
Priority: major Milestone: sage-8.0
Component: symbolics Keywords: symbolic ring
Cc: nbruin, kcrisman, rws Merged in:
Authors: Marcelo Forets Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 29c9c2d (Commits) Commit: 29c9c2d1dcb02cf10afdc6cd13f09a7ea4dcc73e
Dependencies: Stopgaps:

Description

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

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

Change History (21)

comment:1 Changed 2 years ago by mforets

  • Authors set to Marcelo Forets
  • Branch set to u/mforets/22909
  • Commit set to 3093d25c4bece6dcd4aa4dd3b925b67bb5279a45
  • Status changed from new to needs_review

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


New commits:

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

comment:2 follow-up: Changed 2 years ago by 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.

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

comment:3 in reply to: ↑ 2 ; follow-up: Changed 2 years ago by mforets

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.

comment:4 in reply to: ↑ 3 Changed 2 years ago by tscrim

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.

comment:5 Changed 2 years ago by git

  • Commit changed from 3093d25c4bece6dcd4aa4dd3b925b67bb5279a45 to 751d7dfb0eebd1f8f0ecb97dbd5c386187b4f418

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

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

comment:6 Changed 2 years ago by tscrim

  • Cc nbruin kcrisman rws added

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

comment:7 Changed 2 years ago by kcrisman

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.

comment:8 Changed 2 years ago by tscrim

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

comment:9 Changed 2 years ago by mforets

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.

comment:10 Changed 2 years ago by nbruin

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

comment:11 Changed 2 years ago by git

  • Commit changed from 751d7dfb0eebd1f8f0ecb97dbd5c386187b4f418 to 3d508e4120b95a083f5674aa1bb50c78d254e227

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

3d508e4specify exception type

comment:12 Changed 2 years ago by vdelecroix

See also the related #18390

comment:13 Changed 2 years ago by mforets

  • Status changed from needs_review to needs_work

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

comment:14 Changed 2 years ago by git

  • Commit changed from 3d508e4120b95a083f5674aa1bb50c78d254e227 to 5676d2517a13b809becdd1b80f16a1ee88000f51

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

5676d25update docstring

comment:15 Changed 2 years ago by git

  • Commit changed from 5676d2517a13b809becdd1b80f16a1ee88000f51 to f34ac3360cb63b5394ec51986fd49ee99baf8dec

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

comment:16 Changed 2 years ago by mforets

  • Status changed from needs_work to needs_review

comment:17 follow-up: Changed 2 years ago by tscrim

Last little thing from me: this could be done

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

comment:18 Changed 2 years ago by git

  • Commit changed from f34ac3360cb63b5394ec51986fd49ee99baf8dec to 29c9c2d1dcb02cf10afdc6cd13f09a7ea4dcc73e

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

29c9c2dsimplify formatted lated name line

comment:19 in reply to: ↑ 17 Changed 2 years ago by mforets

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.

comment:20 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

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

comment:21 Changed 2 years ago by vbraun

  • Branch changed from u/mforets/22909 to 29c9c2d1dcb02cf10afdc6cd13f09a7ea4dcc73e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.