modpi - initial revision

[fabianlischka]

to fix #2524

see here for an overview and testing: http://nbviewer.ipython.org/7443293

[simonbyrne]

We could be really clever here, and make mod dispatch on the appropriate MathConst types.

[StefanKarpinski]

@simonbyrne: that's already in here :-)

[fabianlischka]

Ok, I think I've incorporated all your comments, and added a test file (and added that to runtests.jl).
(I might have gone a bit over board with the testing (it's the biggest file .jl file in the directory; but it's fast, fortunately)).

Anything else?

[JeffBezanson]

This is extremely impressive --- that level of testing is amazing.

I dislike doing this by calling parse and eval. Does it really need to be done that way? Including a data table as arrays of numbers and strings seems much nicer.

I'm not sure if we need to list these functions under "division functions" in the manual, but I guess it's ok. However help entries in doc/stdlib/base.rst would be great.

[fabianlischka]

Oh, this continuous integration testing is pretty cool!
Interesting that I don't have to specify libm in the call
n = ccall(:__ieee754_rem_pio2, Cint, (Float64,Ptr{Float64}),x,y)
on my machine locally, but it fails on the test there. Anyway, fixed, I hope (using copy, paste, modify...)

[fabianlischka]

@JeffBezanson - thanks, and very good point. My Julia was broken for a bit when I transitioned to OS 10.9, so I did the testing against WolframAlpha in python and then copied and pasted over, that's why there's all this text parsing stuff. I'll refactor it.

As for documentation, I just searched for "mod" and added them there. But happy to put elsewhere, let me see where they'd fit in in base.rst.

[ViralBShah]

@staticfloat is to be thanked for our CI. Since libm almost always gets linked, you do not need to specify it in the ccall line. However, you are not calling openlibm in that case, but most likely the system libm.

The notebook is really nice. I'll probably steal stuff from it for presentations! (Cc: @alanedelman)

[ViralBShah]

Looking through the commit source, this is indeed calling openlibm.

[simonbyrne]

@StefanKarpinski Ah, I missed that, very nice work @fabianlischka.

Probably should document the behaviour though, so users aren't confused when mod(2*pi, pi) != 0.0.

[ivarne]

Users will be confused by floating point behaviour anyway. If you change π to 0.1 and change the constant to something less favorable than 2, you get mod(3*0.1, 0.1) == 2.7755575615628914e-17.

We should teach users to not use == or != on floating point numbers, when they are not working with exact fractions of 2. I would almost argue that they should not be implemented.

[fabianlischka]

To dispatch on the MathConstant type for pi (by type, without runtime penalty) is very nice, but I am ambiguous about it, for these reasons:

• mod(x,pi) is caught and goes to modpi(x), but mod(x,2pi) and mod(x,pi/2) don't
• mod(355,pi_1) != mod(355,pi) (because pi_1 is promoted to Float64; only the RHS == modpi(355))
• ar = [pi,2pi]; mod(355,ar[1]) != mod(355,pi) (because the array is promoted to Float64; only the RHS == modpi(355))

Note that these anomalies go somewhat beyond the usual floating point issues.

I am wondering whether alternatively:
a) one could leave mod(x,y) as it is, but catch y of type MathConstant pi and issue a warning/suggestion to use modpi; or
b) one should modify mod(x,y) to catch y = pi, 2pi, pi/2 (by value, not by type), and dispatch to modpi, mod2pi, modpio2 instead. A bit more of a runtime penalty, obviously.

What do you think?

[StefanKarpinski]

I agree that it does feel a bit fishy. I keep wanting to extend the MathConst thing further, but that road can rapidly lead to insanity. One thing we could do is include an integer coefficient in MathConsts so that 2pi would still be a MathConst, but collapse as soon as anything but integer multiples become involved. But that's an entirely different issue. How about restricting this initial pull request to just adding the modpi function. That's already a huge amount of very important functionality (all packed into a single function with a simple interface, which is the best way). People who need it can use it and everyone else is unaffected.

[JeffBezanson]

I agree; at least for now let's not make this part of any other function.
On Nov 17, 2013 5:46 PM, "Stefan Karpinski" [email protected]
wrote:

I agree that it does feel a bit fishy. I keep wanting to extend the
MathConst thing further, but that road can rapidly lead to insanity. One
thing we could do is include an integer coefficient in MathConsts so
that 2pi would still be a MathConst, but collapse as soon as anything but
integer multiples become involved. But that's an entirely different issue.
How about restricting this initial pull request to just adding the modpifunction. That's already a huge amount of very important functionality (all
packed into a single function with a simple interface, which is the best
way). People who need it can use it and everyone else is unaffected.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4799#issuecomment-28666508
.

[fabianlischka]

So, just to confirm, I'll remove these lines for now, right?

mod(x::Float64, y::MathConst{:π}) = modpi(x)
mod(x::Float32, y::MathConst{:π}) = modpi(x)
mod(x::Int32, y::MathConst{:π}) = modpi(x)
mod(x::Int64, y::MathConst{:π}) = modpi(x)

[StefanKarpinski]

Yes.

[fabianlischka]

I've added modpi etc. documentation to both doc/manual/mathematical-operations.rst and doc/stdlib/base.rst.
Note that I did not manage to build the documentation (I'm getting: make: *** No rule to make target 'helpdb.jl'. Stop., and make: *** No rule to make target 'html'. Stop., etc.)

I'm not aware of anything missing now, so I think you can pull (assuming the help files are ok)

[fabianlischka]

BTW, no need to modify the make file, right? (I notice that andrioni made a few changes to the makefile when exposing the floating point rounding flags)

[JeffBezanson]

I believe test/Makefile needs to be updated if you want it to be possible to run these tests via make test-math-modpi.

[fabianlischka]

oops... seems I segfaulted the gcc build. Was that a fluke, or is there a problem with the last commit? Can anyone have a look? How can I re-run it?

Segmentation fault
*** This error is usually fixed by running 'make clean'. If the error persists, try 'make cleanall'. ***
make[2]: *** [/home/travis/build/JuliaLang/julia/usr/lib/julia/sys.ji] Error 1
make[1]: *** [debug] Error 2
make: *** [install] Error 2

[JeffBezanson]

It's probably not your fault. I've restarted that build.

[fabianlischka]

Bummer, I forgot modpau.