Ticket #10821 (closed defect: fixed)
riemann.pyx gives lots of invalid value in divide warnings
| Reported by: | jason | Owned by: | burcin |
|---|---|---|---|
| Priority: | major | Milestone: | sage-4.7.1 |
| Component: | calculus | Keywords: | |
| Cc: | evanandel, fbissey | Work issues: | |
| Report Upstream: | N/A | Reviewers: | Ethan Van Andel, Karl-Dieter Crisman |
| Authors: | François Bissey, Jason Grout | Merged in: | sage-4.7.1.alpha0 |
| Dependencies: | Stopgaps: |
Description (last modified by evanandel) (diff)
I keep getting Warning: invalid value encountered in divide, and the error happens in these lines in the _generate_theta_array function in calculus/riemann.pyx:
K = np.array(
[-TWOPI / N * sadp * sadp[t] * 1 / (TWOPI * I) *
((dp / adp) / (cp - cp[t]) -
((dp[t] / adp[t]) / (cp - cp[t])).conjugate())
for t in np.arange(NB)], dtype=np.complex128)
Attachments
Change History
comment:1 Changed 2 years ago by jason
- Cc evanandel added
- Owner changed from tbd to burcin
- Component changed from PLEASE CHANGE to calculus
- Description modified (diff)
- Milestone set to sage-4.6.2
comment:3 Changed 2 years ago by jason
Unfortunately, it appears that I really changed something in my patch, or introduced a significant amount of numerical error, as now the doctests give results that are way different than documented.
comment:4 Changed 2 years ago by fbissey
I am not sure what to do with arrays quoted with indices and without, as in:
cp -cp[t]
but I am fairly sure you attached .conjugate() to the wrong expression. I am attaching a corrected patch. I cannot try it right now.
Changed 2 years ago by fbissey
-
attachment
trac-10821-riemann-invalid-divide.2.patch
added
my own version of the patch
comment:5 follow-up: ↓ 6 Changed 2 years ago by evanandel
- Description modified (diff)
This code generates a square array of dimension NB. The 't'th row is generated by (among other things) computing the difference between every element of cp and cp[t]. That's the meaning of
cp - cp[t]
fbissey: It looks like your patch is correct, I'll try it shortly.
Jason: If it interests you, this section of code is preparing a square matrix for nystrom numerical integration. The cp array holds the collocation points, that is the points around the boundary of the figure where the integral is being evaluated. dp contains the derivatives at those points.
computing the square array will of course end up with illegal divisions for the 't'th element of the 't'th row. However, the next line of code:
for i in xrange(NB):
K[i, i] = 1
overwrites the bad elements with 1's. Thus the divisions by zero don't affect the algorithm. (If we have any other divisions by 0, that means that the user has tried to evaluate a self intersecting figure (and gotten very unlucky) for which the algorithm is meaningless anyway.)
We can deal with the zero divides whatever way fits sage style best. If there's an efficient way to not perform those divisions that's fine. Otherwise I have no problems with simply ignoring the warnings.
- Ethan
comment:6 in reply to: ↑ 5 Changed 2 years ago by fbissey
- Status changed from new to needs_work
- Milestone changed from sage-4.6.2 to sage-4.7
Replying to evanandel:
We can deal with the zero divides whatever way fits sage style best. If there's an efficient way to not perform those divisions that's fine. Otherwise I have no problems with simply ignoring the warnings.
I will think about that a little bit but given the context we can indeed ignore these warnings. I think they show up in stderr and don't affect the results of the tests. They are obvious when the tests fail but I am not sure they are otherwise.
comment:7 Changed 2 years ago by fbissey
Do I have the syntax right in thinking that K off-diagonal elements are defined by
K[i,j]= -TWOPI / N * sadp[i] * sadp[j] * 1 / (TWOPI * I) *
((dp[i] / adp[i]) / (cp[i] - cp[j]) -
((dp[j] / adp[j]) / (cp[i] - cp[j])).conjugate())
or do I have i and j mixed up?
comment:8 Changed 2 years ago by kcrisman
Seeing this made me look through riemann.pyx, and it turns out that file could use some general TLC. Esp. with regard to a few doc things and some fairly massive redundancy with complex plots. See #10945.
Changed 2 years ago by evanandel
-
attachment
trac-10821-riemann-invalid-divide.3.patch
added
slight fix
comment:9 Changed 2 years ago by evanandel
- Status changed from needs_work to needs_review
Alright, I wrapped the offending code in calls that tell numpy to ignore those invalid divide warnings then restore the original settings. It seems to fix things well.
comment:11 Changed 2 years ago by kcrisman
- Status changed from needs_review to positive_review
- Reviewers set to Ethan Van Andel, Karl-Dieter Crisman
- Authors set to François Bissey, Jason Grout
Okay, the code seems right, (and slightly refactored but fine), conforms with Numpy specs, passes the tests, and Ethan says it's correct for his algorithm.
comment:13 Changed 2 years ago by jdemeyer
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-4.7.1.alpha0

Ethan, I noticed several things that could be pulled out of the inner loop, and so rewrote this line in the attached patch. However, I don't know this code or the algorithms very well---can you look at the patch?
It appears that the warnings happen when there is a division by zero, as in this test case:
So should we turn off the error checking for this error when doing this operation since at least one entry in cp-cp[t] will be zero? Or should we check for zero denominators and not calculate the fraction with those?