Add `rtol` to `is_real`

[timholy]

trafficstars

Absolute magnitude is affected by problem scaling, but relative magnitude of the real and imaginary components is not. Thus using rtol in preference to tol makes this step more invariant to scale.

This is opt-in to avoid breaking behavior.

One thing to note, this makes the kwarg version the "real" method, and the two-arg just a stub. I can flip this if you prefer.

[PBrdng]

Thanks. This is a good idea. I pushed another commit to update all functions that call is_real.

[PBrdng]

If there's ever a v3 of this package, there are two points that might be worth consideration:

I plan to have a v3 when the work on numerical irreducible decomposition is finished.

• rename tol to atol to make it more consistent with base Julia

This makes sense. In fact, in all other functions tolerances are called atol and rtol. I think the name tol in this case is an artifact from one of the very first version of the software that never has been updated. I think we should do this within this PR, keeping tol as an additional option to not brake existing code.

• ask whether the positional version of this function makes sense now that there are two tolerances (e.g., is there any reason users should expect one ordering vs the other?)

From a numerical analysis viewpoint, rtol should be the preferred one. However, I don't think this question is that important, since is_real is only a heuristic. There is the certify function that can give a computational proof for reality.

One thing I notice now about my own changes: Julia's isapprox uses atol and rtol in an || sense rather than an && sense:

Do you mean that, when both tolerances are given, both conditions should be satisfied for is_real to return true?

[timholy]

Do you mean that, when both tolerances are given, both conditions should be satisfied for is_real to return true?

Right: this PR currently returns true only if all non-nothing conditions are met. But Base.approx returns true if any condition is met, and nothing is not a valid setting. I'm proposing I should change this to make it more consistent with Base.approx, with the exception of atol currently being named tol. OK with you?

If you approve, I'll also make the tol -> atol change (with a fallback of tol) at the same time.

[PBrdng]

Do you mean that, when both tolerances are given, both conditions should be satisfied for is_real to return true?

Right: this PR currently returns true only if all non-nothing conditions are met. But Base.approx returns true if any condition is met, and nothing is not a valid setting. I'm proposing I should change this to make it more consistent with Base.approx, with the exception of atol currently being named tol. OK with you?

Yes, let's do that.

If you approve, I'll also make the tol -> atol change (with a fallback of tol) at the same time.

Thanks, that would save me some time :)

[timholy]

When I run tests locally, they fail, but that's fixed by #651

I think this is otherwise ready to go.

[timholy]

I should add that I've verified that if you back out the changes to the test-suite, the only tests that fail are due to the change in printing of SemialgebraicSetsHCSolver. Thus the deprecations all work.

[PBrdng]

Hi, a sign of life from me: I will review your changes asap, but I'm at a conference this week and so it might take a few more days.