Added differentiation rule for mod2pi

[papamarkou]

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

[johnmyleswhite]

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]

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

[johnmyleswhite]

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]

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 <[email protected]

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
#89 (comment)

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]

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.