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

sage.calculus: Modularization fixes, doctest cosmetics, # needs #35717

Merged
merged 29 commits into from
Aug 5, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jun 4, 2023

📚 Description

Not all of sage.calculus is for symbolic calculus. We change some import statements to lazy_import so that some modules can be loaded even when the symbolic subsystem is not present, or move them into methods.

We also add # needs tags to doctests to enable doctesting of modularized distributions.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

@mkoeppe mkoeppe changed the title sage.calculus: Modularization fixes sage.calculus: Modularization fixes, doctest cosmetics, # optional Jun 19, 2023
@mkoeppe mkoeppe requested a review from orlitzky June 19, 2023 22:25
src/sage/calculus/ode.pyx Outdated Show resolved Hide resolved
sage: A = [exp(-2*pi*i*I/5) for i in J]
sage: s = IndexedSequence(A,J)
sage: s.dct()
sage: A = [exp(-2*pi*i*I/5) for i in J] # optional - sage.symbolic
Copy link
Contributor

Choose a reason for hiding this comment

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

This one really uses the symbolic pi and e, but could easily be rewritten to use some other familiar sequence like the triangular numbers

sage: for i in range(1800): a[2048-i-1] = 0
sage: a.backward_transform()
sage: a.plot().show() # long time (7s on sage.math, 2011)
sage: for i in range(2048): a[i]=float(sin((i*5/2048)**2)) # optional - sage.symbolic
Copy link
Contributor

Choose a reason for hiding this comment

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

float(sin(x)) can be math.sin(x)

@@ -19,7 +19,7 @@
sage: R.<x> = PowerSeriesRing(QQ, default_prec=10)
sage: R.default_prec()
10
sage: cos(x)
sage: cos(x) # optional - sage.symbolic
Copy link
Contributor

Choose a reason for hiding this comment

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

Does using x.cos() bypass the symbolic cosine?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not needs sage.symbolic?

SageMath version 10.1.beta4, Release Date: 2023-06-21
@mkoeppe mkoeppe changed the title sage.calculus: Modularization fixes, doctest cosmetics, # optional sage.calculus: Modularization fixes, doctest cosmetics, # needs Jul 14, 2023
@@ -1,3 +1,4 @@
# sage.doctest: optional - sage.symbolic
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs sage.symbolic would work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed (in all files)

@@ -71,20 +71,20 @@
#
# https://www.gnu.org/licenses/
##########################################################################
from sage.rings.number_field.number_field import CyclotomicField
from sage.functions.all import sin, cos
from sage.misc.lazy_import import lazy_import
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this (importing from all) all right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not enforced for sage.functions yet, but I've changed it

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Documentation preview for this PR (built with commit e1decf3; changes) is ready! 🎉

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

Failed checks seem unrelated with this PR. Otherwise LGTM.

I leave setting positive review to @orlitzky.

@orlitzky
Copy link
Contributor

orlitzky commented Aug 2, 2023

Don't wait for me, I got busy and now all of my sage installs are broken.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 2, 2023

OK. Good wishes for your sage installs!

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 2, 2023

Thanks both!

@vbraun vbraun merged commit d229f45 into sagemath:develop Aug 5, 2023
10 of 13 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Aug 5, 2023
This pull request was closed.
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