Adds limit(expr, v, a) syntax by EigenVector22 · Pull Request #39812 · sagemath/sage

EigenVector22 · 2025-03-28T13:21:28Z

Implemented the limit(expr, variable, value) positional syntax to allow limits with respect to indexed variables or other variables not usable as keyword arguments.

Also, Updated the documentation and added doctests for the new syntax and associated error handling. Ensuring code coverage for the new argument parsing.

Fixes #38761 by allowing limits to be taken with respect to indexed variables like x[0] or other symbolic expressions not usable as keyword arguments.

While testing, the tests passed except the optional fricas ones that were failing before too.

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Got the idea and direction of fixing the issue and the link to the relevant conversations in the given PR: #38780

EigenVector22 · 2025-03-28T13:24:58Z

Hello, @vincentmacri, @nbruin,

can you please review this and
suggest if any thing else needs to be done,

Thanks,

nbruin · 2025-03-28T16:00:13Z

 from sage.misc.latex import latex
 from sage.misc.parser import Parser, LookupNameMaker
 from sage.structure.element import Expression
+from sage.symbolic.expression import Expression


Why this import? It shadows the one above. Figure out which one you need (why not the one that was already there?) and remove the other.

yes you right, the second import just overwrites the 1st one, made the necessary changes
Thanks,

sage.structure.element.Expression and sage.symbolic.expression.Expression aren't the same. Do you have a reason to change which one is imported here? There was probably a good reason why the code here originally imported the abstract base class rather than the concrete one.

This still needs addressing

okay, i guess i probably get this now, since the limit function is operating on symbolic expressions (converted via SR), so the abstract base class is probably sufficient, am i right?

vincentmacri · 2025-03-28T20:45:34Z

I'll leave commenting on the code itself to @nbruin since he wrote the first draft of this.

All I'll add is that I find the usage of "new syntax" versus "old syntax" in the docstrings a bit confusing. It makes it sounds like we plan to deprecate the old syntax, which I don't think we do plan on doing (or should plan on doing unless there's a good reason).

I'm not sure what terms would be better. Maybe just show examples of both without referring to one as being new or old, and just mention in the examples why the syntax you're adding here is preferred/needed in some situations. For example, when I added additional functionality to the subs method for function field elements I made sure to have examples of the different syntax you could use. Given the motivation of this PR being to be able to compute limits where the x=a syntax doesn't work, I would explicitly say in the example for the limit(expr, v, a) syntax when you might prefer to use it. (In retrospect I think the same issue with x=a that you're fixing here applied to what I did there for the subs method, I didn't mention it in the example for the dictionary syntax there because it didn't occur to me that you might have indexed variables or something, although the dictionary syntax should support it.)

EigenVector22 · 2025-03-29T05:01:38Z

Hello Vincent,

Thanks for your feedback on the documentation. I also think that labeling the syntaxes as "new" and "old" maybe could imply we are deprecating x=a, which isn’t the case—both syntaxes are meant to stay supported, as of now unless( as you said) ......

I just wanted to ask that what should be next course of action ?
Should I update the docstrings to like remove the "new" and "old" labels and instead present both syntaxes side-by-side with examples. I will TRY to clarify like when each one of them is appropriate to use

I am thinking to include something like this in the docstrings:
Use limit(expr, x=a) for straightforward variable substitutions, but opt for limit(expr, v, a) when v is an indexed variable like y[1], since keyword arguments can’t directly handle indexed names.

Is this the right way of representation?

Thanks,

nbruin · 2025-03-29T06:46:36Z

I am thinking to include something like this in the docstrings: Use limit(expr, x=a) for straightforward variable substitutions, but opt for limit(expr, v, a) when v is an indexed variable like y[1], since keyword arguments can’t directly handle indexed names.

No, just be neutral:

There are two ways of invoking limit. One can write limit(expr, x=a, <keywords>) or limit(expr, x, a, <keywords>). In the first option, x must be a valid python identifier. Its string representation is used to create the corresponding symbolic variable with respect to which to take the limit. In the second option, x can simply be a symbolic variable. For symbolic variables that do not have a string representation that is a valid python identifier, for instance if x is an indexed symbolic variable, the second option is required.

EigenVector22 · 2025-03-29T09:33:06Z

I have made the necessary changes to the doctests and added examples where necessary, can you please review it.
Thanks,

EigenVector22 · 2025-03-29T17:07:03Z

@nbruin any other feedback regarding the code?

nbruin · 2025-03-29T18:28:59Z

+        is_getitem = hasattr(v.operator(), '__name__') and v.operator().__name__ == '__getitem__'
+        if not (is_getitem and v.operands()):
+            # If it’s not an indexed variable, checking if it’s numeric
+            if v.is_numeric():


certainly not necessary to further differentiate if you know you're going to raise an error

nbruin · 2025-03-29T18:30:04Z

+        if not (is_getitem and v.operands()):
+            # If it’s not an indexed variable, checking if it’s numeric
+            if v.is_numeric():
+                raise TypeError(f"Limit variable must be a variable, not a constant number: {v}")


Just raise a more generic message here. You don't need to give the value that is causing the error. That's in the backtrace.

nbruin · 2025-03-29T18:30:49Z

+        try:
+            a = SR(a)
+        except TypeError:
+            raise TypeError(f"Cannot convert limit point to symbolic ring: {a}")


just let SR(a) raise the error.

nbruin · 2025-03-29T18:33:03Z

@@ -1498,38 +1699,53 @@ def mma_free_limit(expression, v, a, dir=None):

    EXAMPLES::


My expertise does not extend to mma_free_limit so I cannot review that code.

EigenVector22 · 2025-03-29T21:53:35Z

I've applied the suggested changes from the latest review.

Regarding the use of kwargv instead of argv: Initially, I intended to use kwargv since the documentation highlighted it as a
standard practice. However, I later opted for argv, my bad, should my discussed this beforehand
I also removed some unnecessary, bloated lines of code from the PR. I've been working on this for 2–3 weeks, and the build
broke a couple of times during that period. T_T

Everything has now been updated as suggested. Do you have any further feedback?

EigenVector22 · 2025-03-30T10:37:03Z

@vincentmacri any other feedback from your side ,?

nbruin

Please make sure you address all concerns. I think I have reflagged issues that I flagged before and that did not receive a response.

If you disagree with my assessment on any one of the items you can respond with your reasons and then we can take it from there.

nbruin · 2025-03-30T21:37:12Z

 from sage.misc.latex import latex
 from sage.misc.parser import Parser, LookupNameMaker
 from sage.structure.element import Expression
+from sage.symbolic.expression import Expression


This still needs addressing

nbruin · 2025-03-30T21:41:22Z

+
+    if len(args) == 2: # Syntax: limit(ex, v, a, ...)
+        if kwargs: # Cannot mix positional v, a with keyword args
+            raise ValueError(f"Use either limit(expr, v, a, ...) or limit(expr, v=a, ...) syntax. "


Error message should be an uncapitalized phrase describing the problem; not advice. So:

'''
Cannot mix positional specification of limit variable and point with keyword variable arguments
'''

No need for arguments in message: those can be obtained from traceback.

I don't think advice of the form Use either ... or ... is necessary here. Any advice would go after the primary error message.

nbruin · 2025-03-30T21:47:26Z

+    elif len(args) == 0: # Potential syntax: limit(ex, v=a, ...) or limit(ex)
+        if len(kwargs) == 1: 
+            k, = kwargs.keys()
+            if not isinstance(k, str):


this test is not required:

sage: def f(*args,**kwargs): return args, kwargs sage: f(**{10:1}) TypeError: keywords must be strings

nbruin · 2025-03-30T21:47:55Z

+            k, = kwargs.keys()
+            if not isinstance(k, str):
+                 raise ValueError(f"Invalid variable specification in keyword argument: {k} (must be a string)")
+            try:


no need to guard with try. Just let error propagate.

nbruin · 2025-03-30T21:48:35Z

+
+    # Ensuring v is a symbolic expression
+    if not isinstance(v, Expression):
+        try:


no need to guard with "try". Just let it raise on error by itself

nbruin · 2025-03-30T21:50:20Z

+    # Ensuring v is a symbolic expression
+    if not isinstance(v, Expression):
+        try:
+            v = SR(v)


create what you need: use

v=SR.symbol(v)

instead.

@nbruin
This approach is creating an Error:

For the call

limit(x^2 + 5, 5, 10):

v = 5 is an integer, not an instance of Expression

SR.symbol(v) is called with v = 5, but SR.symbol() expects a string to create a symbolic variable. Passing integer like 5 causes a

TypeError: expected string or bytes-like object, got 'sage.rings.integer.Integer'.

I think this error is occuring because the code assumes v can be converted to a symbolic variable directly, but a constant like 5 isn’t a valid limit variable, limits must be taken with respect to a variable (e.g., x), not a constant,

its working fine with this approach:

# Ensuring v is a symbolic expression and a valid limit variable if not isinstance(v, Expression): v = SR(v) if not v.is_symbol(): raise TypeError("limit variable must be a variable, not a constant")

nbruin · 2025-03-30T21:50:41Z

-        v = var(k)
-        a = argv[k]
+    # Check if v is a valid limit variable
+    if not v.is_symbol() and v.is_constant():


you will have a symbol here with the change above, so no reason to check

EigenVector22 · 2025-03-31T15:53:33Z

@nbruin does everything looks good now?

nbruin

OK that looks fine now. I did find it painful in the review process to not have the comments and markings from the previous review available. Part of that might be github's interface. Another component is that you squash your commits in a forced push, destroying history. It might be better to just make successive commits. If you really want to squash before merge, you could squash at the very end to make a branch with a different (more concise) history but with the same effect on the tree.

EigenVector22 · 2025-03-31T19:07:49Z

@nbruin , thanks so much for reviewing the PR thoroughly and providing detailed feedback.
I have tried to incorporate your feedback into my recent PRs.

Regarding the comment on squashing commits with a forced push: I initially thought that working alone on the branch wouldn't be an issue, but I see how it complicates code review. I'll adjust my approach and make successive commits going ahead

Thanks again!

EigenVector22 · 2025-04-01T17:54:35Z

hello, @roed314 can you review and run the workflows if possible,
Thanks,

roed314 · 2025-04-01T18:06:17Z

Done. It still looks like there's something strange going on with force pushes (if you look at the Files Changed you'll see a bunch of changes that are just 10.6 vs 10.6.rc1).

github-actions · 2025-04-01T20:14:59Z

Documentation preview for this PR (built with commit ec6e145; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

vincentmacri · 2025-04-01T21:45:34Z

+    The positional `limit(expr, v, a)` syntax is particularly useful when
+    the limit variable `v` is an indexed variable or another expression
+    that cannot be used as a keyword argument (fixes :issue:`38761`)::


Suggested change

The positional `limit(expr, v, a)` syntax is particularly useful when

the limit variable `v` is an indexed variable or another expression

that cannot be used as a keyword argument (fixes :issue:`38761`)::

The positional ``limit(expr, v, a)`` syntax is particularly useful

when the limit variable ``v`` is an indexed variable or another

expression that cannot be used as a keyword argument

(fixes :issue:`38761`)::

When referring to code, typeset it with two ticks (code) rather than one tick (LaTeX).

You can see here how the formatting will look in the documentation: https://doc-pr-39812--sagemath.netlify.app/html/en/reference/calculus/sage/calculus/calculus#sage.calculus.calculus.limit

vincentmacri

Minor formatting fix

vincentmacri · 2025-05-01T23:25:31Z

I have no idea why giac broke here, nothing you did should be causing this issue as far as I can tell. My best guess is some kind of circular import nonsense. I think I have a possible workaround though.

Right now the code calls giac by (attempting to) import libgiac from sage.libs.giac.giac. Maybe there's a way to use from sage.interfaces.giac import giac instead, as we are doing for fricas? I'm not too sure how this interface works though.

nbruin · 2025-05-02T06:48:59Z

-    a = a._mathematica_init_()
+
+    try:
+        math_expr = expression._mathematica_().name()


this whole routine needs to be reverted.

On a system that does not have mathematica installed, _mathematica_() raises an error, whereas _mathematica_init_() doesn't. This routine should be making a web request. It should not be trying to run mathematica locally.

I'm pretty sure that all edits made to this routine are erroneous. They are certainly outside the stated scope of the pull request. If there is a change to be made to mma_free_limit it needs to happen on a different pull request.

EigenVector22 · 2025-05-02T14:01:37Z

(the error is actually on the line numbered 4016. I don't know why the link below seems to refer to line 4022. If I follow the link, I'm ending up at the correct OSError).

thanks for spotting the error, looking into it now.

vincentmacri · 2025-05-02T17:22:51Z

(the error is actually on the line numbered 4016. I don't know why the link below seems to refer to line 4022. If I follow the link, I'm ending up at the correct OSError).

thanks for spotting the error, looking into it now.

The Inline substitution_reference start-string without end-string error is because you're missing a new line before the code block.

vincentmacri · 2025-05-02T17:28:12Z

(the error is actually on the line numbered 4016. I don't know why the link below seems to refer to line 4022. If I follow the link, I'm ending up at the correct OSError).

thanks for spotting the error, looking into it now.

The Inline substitution_reference start-string without end-string error is because you're missing a new line before the code block.

But I agree with @nbruin, leave the changes to mma_free_limit to another PR. So fixing that error is a moot point for this PR.

vincentmacri · 2025-05-02T17:46:28Z

I think the error with giac occurs on develop, you've just added a test that exposes the error. I think it might be related to recent changes by @orlitzky to how Sage interfaces with giac.

@orlitzky do you have any ideas? Does this code need to change to use the giac interface instead of the giac lib?

orlitzky · 2025-05-02T18:18:22Z

I think instead of # optional - giac, you want # needs sage.libs.giac. Otherwise the tests may still run when giac is installed but the sage interface to it has not been built.

EigenVector22 · 2025-05-03T10:39:02Z

doing this only eliminates the giac failure, but the ones related to fricas still exists, my bad, I by mistake force pushed T_T

vincentmacri

Looks good, one minor nitpick that you can take or leave.

Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>

EigenVector22 · 2025-05-03T20:20:45Z

@vincentmacri this is so strange, before incorporating your suggestion all the CIs successfully passed, now one of the CIs is failing, and i don't think that this is anything related to your suggestion that i committed? could there be any problem with the CIs ? do we need to run them again?

vincentmacri · 2025-05-03T21:35:15Z

@vincentmacri this is so strange, before incorporating your suggestion all the CIs successfully passed, now one of the CIs is failing, and i don't think that this is anything related to your suggestion that i committed? could there be any problem with the CIs ? do we need to run them again?

I think it is a problem with the CI. You can try running that test locally with sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/doctest/test.py. If it fails check whether or not it also fails on develop.

This reverts commit 182ece1.

nbruin · 2025-05-05T22:50:40Z

The CI hiccup seems to have resolved itself. I'm OK with the present state:
ec6e145
if @vincentmacri or @dimpase agree, please put the state to "positive" so that the PR can be merged. Congratulations on getting this done! I'm sure it's been an instructive experience :-).

EigenVector22 · 2025-05-06T08:45:45Z

Thank you, @nbruin for all the intense code reviews and feedbacks, and thanks to @vincentmacri and @dimpase for all the guidance and your time. I've learned a lot through this PR.

sagemathgh-39812: Adds limit(expr, v, a) syntax Implemented the limit(expr, variable, value) positional syntax to allow limits with respect to indexed variables or other variables not usable as keyword arguments. Also, Updated the documentation and added doctests for the new syntax and associated error handling. Ensuring code coverage for the new argument parsing. Fixes sagemath#38761 by allowing limits to be taken with respect to indexed variables like x[0] or other symbolic expressions not usable as keyword arguments. While testing, the tests passed except the optional fricas ones that were failing before too.       ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies Got the idea and direction of fixing the issue and the link to the relevant conversations in the given PR: sagemath#38780    URL: sagemath#39812 Reported by: Ashutosh Rajora Reviewer(s): Ashutosh Rajora, Dima Pasechnik, nbruin, Vincent Macri

github-actions Bot added the s: needs review label Mar 28, 2025

nbruin reviewed Mar 28, 2025

View reviewed changes

Comment thread src/sage/calculus/calculus.py Outdated

EigenVector22 force-pushed the fix-limit-syntax-38761 branch from 28b5916 to f468ab1 Compare March 28, 2025 16:22

EigenVector22 force-pushed the fix-limit-syntax-38761 branch 2 times, most recently from 05b3f07 to e806bf7 Compare March 29, 2025 09:30

nbruin reviewed Mar 29, 2025

View reviewed changes

Comment thread src/sage/calculus/calculus.py Outdated

nbruin suggested changes Mar 29, 2025

View reviewed changes

EigenVector22 force-pushed the fix-limit-syntax-38761 branch from e806bf7 to 40a04c1 Compare March 29, 2025 21:41

EigenVector22 requested a review from nbruin March 29, 2025 22:43

nbruin suggested changes Mar 30, 2025

View reviewed changes

EigenVector22 force-pushed the fix-limit-syntax-38761 branch from 40a04c1 to 8858361 Compare March 31, 2025 02:27

nbruin approved these changes Mar 31, 2025

View reviewed changes

Comment thread src/sage/calculus/calculus.py

vbraun force-pushed the develop branch from 8a8453f to b8f98e7 Compare March 31, 2025 22:55

EigenVector22 force-pushed the fix-limit-syntax-38761 branch from 8858361 to e74d80f Compare April 1, 2025 17:49

vincentmacri reviewed Apr 1, 2025

View reviewed changes

vincentmacri suggested changes Apr 1, 2025

View reviewed changes

nbruin reviewed May 2, 2025

View reviewed changes

Revert mma_free_limit (code and docstring) to develop version

dba0423

EigenVector22 force-pushed the fix-limit-syntax-38761 branch from b42232e to dba0423 Compare May 3, 2025 10:31

Now using '# needs sage.libs.giac' for doctests per orlitzky suggestion

a48ed9d

Merge branch 'develop' into fix-limit-syntax-38761

79a5a38

EigenVector22 requested review from nbruin and vincentmacri May 3, 2025 14:49

vincentmacri added the c: calculus label May 3, 2025

vincentmacri reviewed May 3, 2025

View reviewed changes

Comment thread src/sage/calculus/calculus.py

Update src/sage/calculus/calculus.py

182ece1

Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>

Revert "Update src/sage/calculus/calculus.py"

ec6e145

This reverts commit 182ece1.

EigenVector22 requested a review from dimpase May 4, 2025 19:10

dimpase approved these changes May 5, 2025

View reviewed changes

dimpase added s: positive review and removed s: needs review labels May 5, 2025

vbraun merged commit 4dfd788 into sagemath:develop May 11, 2025
22 checks passed

github-actions Bot removed the s: positive review label May 11, 2025

EigenVector22 deleted the fix-limit-syntax-38761 branch March 7, 2026 14:15

		@@ -1498,38 +1699,53 @@ def mma_free_limit(expression, v, a, dir=None):

		EXAMPLES::

Uh oh!

Conversation

EigenVector22 commented Mar 28, 2025

📝 Checklist

⌛ Dependencies

Uh oh!

EigenVector22 commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincentmacri commented Mar 28, 2025

Uh oh!

EigenVector22 commented Mar 29, 2025

Uh oh!

nbruin commented Mar 29, 2025

Uh oh!

EigenVector22 commented Mar 29, 2025

Uh oh!

EigenVector22 commented Mar 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EigenVector22 commented Mar 29, 2025

Uh oh!

EigenVector22 commented Mar 30, 2025

Uh oh!

nbruin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EigenVector22 commented Mar 31, 2025

Uh oh!

nbruin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EigenVector22 commented Mar 31, 2025

Uh oh!

EigenVector22 commented Apr 1, 2025

Uh oh!

github-actions Bot commented Apr 1, 2025 •

edited

Loading

vincentmacri commented May 2, 2025 •

edited

Loading

EigenVector22 commented May 3, 2025 •

edited

Loading