-
-
Notifications
You must be signed in to change notification settings - Fork 510
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
Make Li symbolic and work with complex input #3401
Comments
comment:1
No version
|
Attachment: Li(x).patch.gz |
Attachment: Lix_2.patch.gz |
comment:3
Attachment: Lix_3.patch.gz Sorry in advance to the reviewer and release manager, but I couldn't figure out how to flatten the patches without applying them. |
Reviewer: Mike Hansen |
Author: Michael Yurko |
comment:6
I've added a patch which folds the above patches together and deprecates the eps_rel and err_bound parameters so that code that uses them won't "break", but will get a deprecation warning. I'm okay with myurko's changes so if someone could sign off on the deprecation warning, that'd be great. |
comment:7
Overall looks good, but there should be at least one doctest for the new DeprecationWarnings (I think this was agreed upon somewhere on sage-devel), and there should also be documentation that this actually fulfills the ticket - namely, to extend Li to complex input! It certainly does, but I have no idea if the output is correct (I assume it is):
Something like that should be added. |
Attachment: trac_3401.patch.gz |
comment:8
I've put up a new patch which address the above concerns. |
comment:9
Attachment: trac_3401-reviewed.patch.gz Looks good - sometimes slower, sometimes faster, but it's much better to have the complex functionality than worry about the rest. I removed an auxiliary function which only existed to allow the previous implementation. Positive review! |
Changed reviewer from Mike Hansen to Mike Hansen, Karl-Dieter Crisman |
Changed reviewer from Mike Hansen, Karl-Dieter Crisman to Mike Hansen, Karl-Dieter Crisman, Burcin Erocal |
comment:10
Sorry to come in this late to the discussion, but this needs more work. The
You can get the precision from the argument provided by the user. If the user needs a higher precision, they should explicitly convert the argument to a higher precision, for example by using We should also start converting these to proper symbolic functions that remain symbolic on exact input, but that can be left to another ticket. |
comment:24
Replying to @benjaminfjones:
There's also "European Li" (for the offset one) IIRC... ;-) |
comment:25
I will go with whatever the sagemath intelligentsia thinks appropriate for the number and name of any aliases. And I had better read up on coding conventions before my next efforts :) |
comment:26
Don't worry about the coding conventions, some of them are unwritten and some of them are subtle. I found a few doctest errors after running Change 9 to 10:
simple change,
This one is more mysterious:
|
comment:27
Replying to @sagetrac-martinx:
Well, with names put into the global namespace, the most important thing is that tab completion is likely to suggest you what you're looking for, i.e., the prefix of each name matters. So in this case I think a single instance of With Sage 5.3.beta0, I currently get:
(Of course also the docstring for e.g. |
comment:28
(Also on top of the reviewer patch: ;-) )
|
comment:29
Nice theorem: \exists x : (
|
comment:30
More nitpicking: |
comment:31
Replying to @nexttime:
... or rather |
comment:32
Replying to @benjaminfjones:
and I am a slow learner anyway :)
Agreed.
The function can be removed since it was just a convenience one to support the original li and Li.
The function randomly selects from a list of all Pynac functions, to which is now added Li, and so now the functions chosen have changed. The docs state that it will often raise an error because it tries to create an erroneous expression. In this case it is trying to pass a complex expression to Maxima. Trial and error and got a return result setting the seed to 1. When I have worked out how to merge patches I'll post a revised patch for review that addresses all comments made so far. |
comment:33
Replying to @nexttime:
Both seem to be allowed: http://www.sagemath.org/doc/developer/sage_manuals.html#chapter-sage-manuals-links |
Revised Li including reviewer patch |
comment:34
Attachment: trac_3401.v2.patch.gz I think v2 addresses all comments made so far and includes reviewer patch. ptestlong passes I think; I had some issues with sagedoc.py but that now passes all tests. Too tired to run ptestlong yet again........... |
comment:35
There are lots of lines touched in I looked over the command line and HTML docs for Li and they look good. Running tests now. |
comment:36
I see, it's just whitespace at the beginnings of lines. I guess that doesn't get highlighted by trac when you view the patch. Anyway, I think it's a good thing to clean up trailing whitespace, but you'll have to watch out that touching so many lines of the file doesn't cause conflicts with other patches that modify If leif is happy with the patch, I'll give it a positive review. |
Changed author from Michael Yurko to martinx |
comment:37
Replying to @benjaminfjones:
I got carried away with strip trailing whitespace command in Geany, in response to the previous review comments. Will try to be more restrained next time ;-) |
comment:38
Patches apply cleanly on top of those at #11143 on top of sage-5.3.beta0. All long tests pass. I think this is ready to go. |
This comment has been minimized.
This comment has been minimized.
comment:39
Ok, I'm giving the most recent patch a positive review. If someone can quickly review the small, most recent fix at #11143, perhaps both of these tickets can be closed in sage-5.3. |
Changed keywords from none to Li, log, integral, symbolics |
Changed author from martinx to Martin Cross |
Merged: sage-5.3.rc0 |
Make Li symbolic and work with complex input
Just use mpmath and the ideas from #11143. Probably will have to do a little work to keep the doctests from earlier, maybe deprecate a keyword or two related to precision.
Here is some example code from M. Yurko that explains how to do this.
I think something based on this should be put into the Li function itself.
Apply attachment: trac_3401.v2.patch to the Sage library.
Depends on #11143
CC: @sagetrac-myurko @benjaminfjones
Component: symbolics
Keywords: Li, log, integral, symbolics
Author: Martin Cross
Reviewer: Mike Hansen, Karl-Dieter Crisman, Burcin Erocal, Benjamin Jones
Merged: sage-5.3.rc0
Issue created by migration from https://trac.sagemath.org/ticket/3401
The text was updated successfully, but these errors were encountered: