Skip to content

Conversation

@RainerHeintzmann
Copy link
Contributor

Hello,
as discussed on discourse, differentiation of cispi fails, which is due to copysign being non differentiable. This problem is avoided by defining a rule for sincospi in fastmath_able.jl. Note the the sign was moved from \pi to the sine term (sinpix), to warrant that \pi does not enforce Float64 as result type, which -\pi would do.
Hope that the edit is acceptable, but happy to adapt, if needed.

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you bump the minor version number also,
so can tag a new release?

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021

Codecov Report

Merging #497 (6095e40) into master (0d55c54) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #497   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files          22       22           
  Lines        2350     2351    +1     
=======================================
+ Hits         2306     2307    +1     
  Misses         44       44           
Impacted Files Coverage Δ
src/rulesets/Base/fastmath_able.jl 98.30% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d55c54...6095e40. Read the comment docs.

@oxinabox
Copy link
Member

Looks like it need a guard behind a if VERSION >= v1.6, since sincospi was added in 1.6.

@mcabbott mcabbott merged commit 47df5cf into JuliaDiff:master Aug 12, 2021
@mcabbott
Copy link
Member

Thanks!

@roflmaostc
Copy link

One thing it noticed and was wondering about.

Why do we test with Float64 and not with Float32? For example, this pi type change we would not discover by checking with a Float64. But we would do, with checking a Float32.

@mcabbott
Copy link
Member

I think the reason is that finite differences has larger errors there. But I agree this will miss many type promotion bugs.

The rrule from @scalar_rule should now have projectors, which will correct this. But still not ideal to convert twice.

It would be possible to explicitly test I suppose. Testing could (piratically) turn such projection off, or make it an error. And it could convert all Float64 to Float32 just to check types, even if it doesn't do finite differences? Would have to dig quite a bit in ChainRulesTestUtils.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants