search

JuliaMath / Calculus.jl

Calculus functions in Julia
Other
278 stars 76 forks source link

Added differentiation rule for mod2pi #89

Open papamarkou opened 8 years ago

papamarkou commented 8 years ago

See JuliaDiff/ForwardDiff.jl#113, cc @jrevels, @mlubin, @eford.

johnmyleswhite commented 8 years ago

This one seems subtle since this function only leaves x unchanged if you already accept the relevant equivalence classes. Probably still right, but I'd be interested to know if we're ok with this producing the "wrong" answer if you don't embrace the appropriate equivalence relation.

papamarkou commented 8 years ago

Good point. Is the name of the function (mod2pi) a sufficient logical guard against picking an inappropriate equiv relation?

johnmyleswhite commented 8 years ago

That's ok with me. I'm mostly interested in how we think about testing these things. For example, if you used finite differencing to compare the results of symbolic differentiation, I think you'd get very strange disagreements for points that cross the 2pi boundary.

Would be interested to know what @simonbyrne thinks.

eford commented 8 years ago

Is there a way that when you load the Calculus package, it could append notes like this one to the relevant docstrings?

If not, then I'd think a comment in the code and a mention in the package documentation would do the trick.

Thanks, Eric

On Wed, Mar 30, 2016 at 12:03 PM, John Myles White <notifications@github.com

wrote:

That's ok with me. I'm mostly interested in how we think about testing these things. For example, if you used finite differencing to compare the results of symbolic differentiation, I think you'd get very strange disagreements for points that cross the 2pi boundary.

Would be interested to know what @simonbyrne https://github.com/simonbyrne thinks.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/johnmyleswhite/Calculus.jl/pull/89#issuecomment-203504486

Eric Ford Professor of Astronomy & Astrophysics Center for Exoplanets & Habitable Worlds Center for Astrostatistics Institute for CyberScience Penn State Astrobiology Research Center Pennsylvania State University

papamarkou commented 8 years ago

Would it be reasonable to simply update the docstrings, as Eric suggested, and otherwise add the mod2pi() method as it stands? If there is still uncertainty as to what the optimal solution would be, shall we merge this for now, given that the functionality of this PR is needed? We can always revise it in due course, while being able to make use of the function in the meantime.