Closed 4bc58ef7-7549-4cd5-991f-2361f10b780c closed 15 years ago
Someone needs to turn this into a proper patch.
Cheers,
Michael
Attachment: wigner.py.gz
The wigner.py file has just been updated so that it passes all tests in sage-4.0 since the new symbolic manipulations return slightly different resutls.
Could someone turn this into a patch and review this please?
I am attaching a patch (against 4.0). I had to make some trivial changes to the original code to get this to play nicely within Sage. A couple of issues remain: (a) doctest coverage is not at 100% since two helper functions are not doctested, (b) the docs need to be rest-ified, and (c) the docs should be added to the reference manual.
So I'm leaving this as "needs work".
Attachment: trac_5996.patch.gz
Thanks for converting this into a patch.
Concerning (a): doctest coverage is infact 100%. The functions test_calc_factlist() and test_bigDeltacoeff() are the doctest functions for _calc_factlist() and _bigDeltacoeff(), respectively. Unfortunately, putting the doctests into the original functions causes a exception. If this can be fixed (maybe removing the underscores, but they are supposed to be private), then the doctests can be merged and the test functions deleted.
Concerning (b), (c), I would be happy to help, but I do not know sage well enough. Any help appreciated.
Attachment: 12428.patch.gz
All functions now have doc tests, redundant functions test_calc_factlist() and test_bigDeltacoeff() have been removed.
I want to merge this but the docstrings are not ReST-ified. Can we get a docstring update and a positive review, Alex?
Author: Jens Rasch
Added path to remove all Latex formulas and replace them with text formulas in accordance with the ReST system.
Attachment: 12429.patch.gz
OK, I finally managed to get around to looking at this. There were still some ReST issues, which I fixed in the latest patch trac_5996_doc.patch
. I also added the documentation to the reference manual.
If someone can double-check my patch, we're good to go.
The patches need to be applied in the order they appear on the ticket.
I'm reviewing the coding style and docstring formatting. Here are some problems I notice with the coding style:
Wigner3j
, ClebschGordan
, _bigDeltacoeff
, Racah
, Wigner6j
, Wigner9j
, Gaunt
.def _bigDeltacoeff(aa,bb,cc,prec=None):
it should be written as follows:
def _big_delta_coeff(aa, bb, cc, prec=None):
which makes use of white space so it doesn't look like code is squashed together. Another example, don't do this
if(int(aa+bb-cc)!=(aa+bb-cc)):
Instead, write it like this:
if int(aa + bb - cc) != (aa + bb - cc):
This means that the whole module needs to be reformatted to make use of white space.
If no one beats me to it, I'll upload a patch to address the issues I raised above.
Could I just suggest that we merge this into Sage as is, since IMHO it is a piece of functionality that should be of interest to a number of computational physicist and chemists.
Just to be clear: I have no problem with anyone improving the code. I personally would like to unmerge my ticket that hacked out the latex formatting as soon as the notebook functionality can deal better with latex formatted doc strings. It might also be of interest to reimplement the algorithms in Cython (although I am unsure about the speed up one would get). However, all this could be done in later tickets.
Replying to @sagetrac-jrasch:
Could I just suggest that we merge this into Sage as is, since IMHO it is a piece of functionality that should be of interest to a number of computational physicist and chemists.
Yes, it would be good to have this merged in Sage 4.1.1. But as they now stand, the patches do not conform to a number of coding conventions in addition to conventions regarding docstring formatting.
Jens, thank you for your contribution, but we cannot import the functions with their current camel case names to the top level Sage namespace. Once we do that, for backward compatibility we'd have to provide the functions with those names and deprecation notices for at least 6 months. The effort to fix the names now is much less than the effort that will go into maintaining this afterwards. Thus I agree with Minh, this ticket still needs work.
I didn't check all the issues raised by Minh, since there are quite a few patches, but I hope someone will put in the work to improve the patches so they can be included in 4.1.1. Since Minh volunteered in comment:10, I don't think this will be a problem.
Attachment: trac_5996_doc.patch.gz
OK, I think I fixed all the issues that Minh pointed out, and replaced my latest patch.
In reply to Jens' comment about latex: I have fixed things so that the docstrings do have the nice latex expressions in them, which means that the pdf and html versions of the reference manual will have nicely typeset formulae.
Replying to @aghitza:
OK, I think I fixed all the issues that Minh pointed out, and replaced my latest patch.
I appreciate that you've taken the time to make the code and docstring conform to the coding styles. It's a tedious task that causes short-term pain. But the long-term benefit is that at least the Sage library is standardized on a coding convention. Your new patch should make the reviewing process easier for me.
In reply to Jens' comment about latex: I have fixed things so that the docstrings do have the nice latex expressions in them, which means that the pdf and html versions of the reference manual will have nicely typeset formulae.
Note that with Sage 4.1, the HTML version of the reference manual is a bit broken. The HTML version of the reference does build successfully. However, if the docstring for a function or class uses the ".. MATH::" tag, then it won't render in the generated HTML version. That is, when you use a web browser to view the corresponding HTML page that documents the function, anything typeset within the ".. MATH::" tag won't show up. You can get a quick glimpse of the rendered math expression, but you have to refresh the page every second. And the math expression would only be displayed for a fraction of a second. This issue was reported at this sage-devel thread and the corresponding ticket is #6512.
Attachment: trac_5996-reviewer.patch.gz
reviewer patch
I've made some minor reformatting of the docstrings and fixed references so that there is only one reference defined for a paper cited in the module. That way, we don't get a lot of warnings about duplicate citations when building the reference manual. That is, only define one reference. Afterwards, if you need to cite that reference, use the convention for citing it and don't define the same reference locally for a function. Some functions use <>
when it should be !=
. At least in Python 2.6.x, using <>
is OK and is kept in that version for backward compatibility. We should now only use !=
. These issues, and some minor ones, are addressed in the reviewer patch trac_5996-reviewer.patch
. Positive review to the ticket as a whole. Patches should be merged in this order:
trac_5996.patch
12428.patch
12429.patch
trac_5996_doc.patch
trac_5996-reviewer.patch
Reviewer: Alex Ghitza, Minh Van Nguyen
Just to let people know, this has been merged in sage-4.1.1-alpha0. I can't close this ticket because I don't have the privilege to do so. Sorry, folks :-(
Just wanted to say thanks for your help and work.
Merged: sage-4.1.1-alpha0
Just as a followup, note that
sage: gaunt(1,1,1,0,1,-1)
0
sage: gaunt(int(1),int(1),int(1),int(0),int(1),int(-1))
1/2*sqrt(3)/sqrt(pi)
This was noted at this ask.sagemath question. I've opened #14735 for this.
Python file for calculating Wigner 3j, 6j, 9j, Clebsch-Gordan, Racah and Gaunt coefficients (integrals over 3 spherical harmonics) exactly.
Has already received some positive review at http://groups.google.com/group/sage-devel/browse_thread/thread/ca312a02de54553e
CC: @certik @jdchristensen
Component: calculus
Author: Jens Rasch
Reviewer: Alex Ghitza, Minh Van Nguyen
Merged: sage-4.1.1-alpha0
Issue created by migration from https://trac.sagemath.org/ticket/5996