Skip to content

Add warning in LazyPowerSeriesRing.taylor() documentation #39880

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

AdityaK1729
Copy link
Contributor

@AdityaK1729 AdityaK1729 commented Apr 5, 2025

part 1 of #39838
As the title says, adds another warning in the LazyPowerSeriesRing.taylor() documentation

📝 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

@AdityaK1729 AdityaK1729 marked this pull request as draft April 5, 2025 08:47
@AdityaK1729 AdityaK1729 marked this pull request as ready for review April 5, 2025 09:10
@AdityaK1729
Copy link
Contributor Author

Can someone review changes, thanks

@AdityaK1729
Copy link
Contributor Author

Can someone review changes, thanks

@user202729

@EigenVector22
Copy link
Contributor

EigenVector22 commented Apr 5, 2025

hello, i think that you have only implemented the second part of the proposed solution and that is adding the warning which is definitely helpful, but you didn't implement the main fix, that is checking coefficients up to the halting precision during construction which would involve computing coefficients up to the default halting precision at initializing time and then verifying whether they can be expressed in the base ring or not, also by doing this we won't be disturbing the lazy evaluation behavior for higher precision terms.

This is what i suspect is the case here, as the issue label also pointed out it's an enhancement issue.

Of course, the final implementation details could be confirmed by the maintainers,
Thanks,

Copy link

github-actions bot commented Apr 5, 2025

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

@AdityaK1729
Copy link
Contributor Author

@user202729 @DaveWitteMorris can you please review the PR, thanks :)

Comment on lines 3041 to 3042
:issue:`39838` For inputs with precision greater than the default,
this does not check that the function is well-defined in the base ring::
Copy link
Contributor

@user202729 user202729 Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:issue:`39838` For inputs with precision greater than the default,
this does not check that the function is well-defined in the base ring::
For inputs with precision greater than the default,
this does not check that the function is well-defined in the base ring (:issue:`39838`)::

As is, the capitalization is incorrect since For is not at the start of a sentence.

image

Also the explanation is misleading, first, the input always have infinite precision, and second this never check the function is well-defined even if a coefficient less than halting_precision does not belong to the base ring.

And this doesn't completely fix the issue, consider change the initial message to avoid https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests

Co-authored-by: user202729 <25191436+user202729@users.noreply.github.com>
@@ -3036,6 +3036,18 @@ def taylor(self, f):
For inputs as symbolic functions/expressions, this does not check
that the function does not have poles at `0`.

.. WARNING::

For inputs with precision greater than the default,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very misleading - the default precision is infinity.

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

Successfully merging this pull request may close these issues.

4 participants