Implement isapprox and ==

[aytekinar]

Implemented isapprox and == between Numbers and Polys.

Closes #96.

[jverzani]

Seems useful to have.

[jverzani]

It isn't exported, but Base.rtoldefault(T,S) does this. Maybe that should be used?

[aytekinar]

I hadn't known Base.rtoldefaults before. Seems like a good function! I just implemented by reading what is available with ?isapprox. But, then, we should remove import Base.eps and any method overloads related to eps to have consistency in the package. What do you think?

Since Polynomials already introduces eps for our use (for roots and gcd), I wanted to use eps and the documentation provided to us (what reltol is for isapprox).

[jverzani]

I would like to remove the Base.eps overload.

In gcd we have this check: reduce(&, (@compat abs.(b.a)) .<=2*eps(S)). Which requires defining eps(Integer). I'd much rather see something like b ≈ zero(S) there. This would even work with Integer types, as rtoldefaults(Integer, Integer) = 0.

[jverzani]

Do you want to do this within this PR or should we merge this in, and do that in another?

[aytekinar]

I can do that. How should we implement truncate and chop then?

[aytekinar]

... and should isapprox allowed to be throw as in isapprox([1,2], [1,2,3]) (DimensionMismatch), or should it just return false? I am asking this, because the way I have implemented is allowed to throw. Then, you have problems in pade. But I can just return false by checking against degree(p1) == 0 when it is with respect to Numbers, or against degree(p1) == degree(p2) when it is between two Polys. What do you think? Or, we can use chop and padd the vectors to have the same dimension, and check using isapprox.

[jverzani]

Hmm, there is some thinking to do here. My first inclination would to truncate before calling isapprox, and then return false if they aren't the same. I don't really like throwing an error here, as that seems unexpected, especially as we allow the comparison of polys to numbers. Likely it is beset to merge this in first and discuss these issues separately.

[jverzani]

These could use a doc string? This efficiently compares p1 with Poly([n1]). There are other norms that might be expected here.

[aytekinar]

Checking back with ?isapprox, you are right. The documentation says there is a keyword argument norm::Function, but since it is not listed in the signature of the function in the documentation, I just disregarded it.

Thanks for the important notes!

[aytekinar]

@jverzani how does it look now?

• isapprox is implemented the way you would like it to be, i.e., without throwing,
• gcd is type-stable now,
• Removed all occurrences of eps to Base.rtoldefault,
• truncate, chop and roots are using Base.rtoldefault and isapprox against zeros of the corresponding types.

Sorry for merging them all, but I thought they were all related.

[jverzani]

I'll merge when the tests pass. Thanks again. My one concern is the change of tolerance for chop and truncate from 1e-16 to 1e-8. That might be unexpected for somebody out there. But let's see. We can roll that back as needed. Otherwise, this seems to make things much more systematic.

[aytekinar]

Correct --- but the tests are passing in 0.4.7 and 0.5.0. So, I did not bother to push the changes. In that case, as you said, we can roll back and add the test accordingly so that it will not break again.

Now, as you might have already noticed, I could also shrink the code for truncate thanks to Base.rtoldefault.

Thanks for the tip on rtoldefault --- hadn't known it before :)

[jverzani]

Thanks again.