JuliaApproximation / DomainSets.jl

A Julia package for describing domains as continuous sets of elements
MIT License
72 stars 12 forks source link

`atol` and `rtol` in `isapprox` #122

Closed jishnub closed 3 months ago

jishnub commented 1 year ago

This adds atol and rtol parameters to the isapprox defined here. This method is committing type-piracy here (and perhaps harms pre-compilation, as it is overwriting the other method), so perhaps this should be removed in a future breaking version. The breakage comes from the fact that the isapprox defined in IntervalSets throws an error when comparing closed and open intervals, whereas this version doesn't.

codecov[bot] commented 1 year ago

Codecov Report

Base: 86.09% // Head: 86.10% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (90f5ebd) compared to base (8bce5e6). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #122 +/- ## ======================================= Coverage 86.09% 86.10% ======================================= Files 31 31 Lines 2496 2497 +1 ======================================= + Hits 2149 2150 +1 Misses 347 347 ``` | [Impacted Files](https://codecov.io/gh/JuliaApproximation/DomainSets.jl/pull/122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation) | Coverage Δ | | |---|---|---| | [src/domains/interval.jl](https://codecov.io/gh/JuliaApproximation/DomainSets.jl/pull/122/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation#diff-c3JjL2RvbWFpbnMvaW50ZXJ2YWwuamw=) | `94.68% <100.00%> (+0.02%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

daanhb commented 1 year ago

Throwing an error when comparing open and closed domains seems like a sensible thing to do. Type-piracy aside, I think we should update DomainSets accordingly.

jishnub commented 1 year ago

Removing this method from DomainSets would make such a comparison throw an error, as the method in IntervalSets does that already. However, this is technically breaking, so ideally this should be removed in the next minor release

daanhb commented 1 year ago

Sure, we can make version 0.6. I think @dlfivefifty is looking at this issue in #123 so I won't touch anything.

jishnub commented 1 year ago

Perfect. I would suggest making a v0.5 release with this PR, so that isapprox(a, b; rtol), which works currently after loading IntervalSets, is not broken after subsequently loading DomainSets although the method is overwritten. Following this, the method may be removed from DomainSets altogether in v0.6

Edit: Sorry, it seems the method is not overwritten after all. Only the isapprox(a, b) one is overwritten, not the one with kwargs. In that case, this PR might not be necessary.

dlfivefifty commented 1 year ago

Note this is now in IntervalSets.jl: https://github.com/JuliaMath/IntervalSets.jl/pull/125

I think throwing an error makes no sense; see my PR https://github.com/JuliaMath/IntervalSets.jl/pull/129

daanhb commented 3 months ago

Meanwhile, that definition seems to have disappeared. I have also removed the tests for approximate equality of intervals, since that is defined and tested in IntervalSets. We still have to implement/improve isapprox more generally for domains.