Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use inf, sup (or bounds) instead of .lo and .hi #531

Merged
merged 2 commits into from
May 29, 2022
Merged

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented May 28, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #531 (b19f757) into 1.0-dev (49f4806) will increase coverage by 0.03%.
The diff coverage is 93.00%.

@@             Coverage Diff             @@
##           1.0-dev     #531      +/-   ##
===========================================
+ Coverage    85.40%   85.44%   +0.03%     
===========================================
  Files           34       34              
  Lines         1782     1814      +32     
===========================================
+ Hits          1522     1550      +28     
- Misses         260      264       +4     
Impacted Files Coverage Δ
src/decorations/intervals.jl 83.33% <0.00%> (ø)
src/intervals/interval_operations/extended_div.jl 100.00% <ø> (ø)
src/plot_recipes/plot_recipes.jl 0.00% <0.00%> (ø)
src/decorations/functions.jl 81.76% <80.00%> (ø)
src/display.jl 82.91% <91.66%> (+0.33%) ⬆️
src/intervals/interval_operations/numeric.jl 97.61% <91.66%> (ø)
src/intervals/arithmetic/basic.jl 91.52% <91.83%> (ø)
src/intervals/arithmetic/trigonometric.jl 96.75% <97.27%> (+0.18%) ⬆️
src/intervals/arithmetic/power.jl 97.10% <97.43%> (ø)
src/bisect.jl 55.55% <100.00%> (ø)
... and 14 more

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 49f4806...b19f757. Read the comment docs.

@lbenet
Copy link
Member Author

lbenet commented May 28, 2022

There are few instances left; by some reason, using inf/sup produces errors in the tests. So for the time being, I think this is ok...

@lbenet lbenet requested a review from lucaferranti May 28, 2022 01:30
@lbenet lbenet added the 1.0 Planned for the major 1.0 release label May 28, 2022
Copy link
Member

@lucaferranti lucaferranti left a comment

Choose a reason for hiding this comment

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

in general LGTM.

If I understand correctly the missing cases are

  1. in trigonometric.jl, @round(F, half_pi(F).lo, atan(ylo, xlo)) we cannot use inf inside the round macro (at least without adding inf(a, RoundingMode) but feels too pedantic

  2. bounds(x) = x.lo, sup(x) the reason for this (which is I guess also the reason for always normalising -0.0 to 0.0 when constructing the interval instead of directly "correctly" normalising lower and upper bound. Is that having -0.0 around leads some ambiguities errors in atan(y, x) computation (returns π instead of or viceversa). Given that the current thing works and given how idiotic the whole -0.0 vs 0.0 issue is in the first place, I would say this is fine, since it works

  3. A couple of instances in the examples folder, but given those are probably outdated anyway I guess it doesn't matter

@Kolaru
Copy link
Collaborator

Kolaru commented May 28, 2022

  1. in trigonometric.jl, @round(F, half_pi(F).lo, atan(ylo, xlo)) we cannot use inf inside the round macro (at least without adding inf(a, RoundingMode) but feels too pedantic

F(inf(half_pi(F)), atan(ylo, xlo, RoundUp)) should do the trick (I don't want to mess with the PR while you are rebasing :))

@lbenet
Copy link
Member Author

lbenet commented May 28, 2022

The last commit takes into account @Kolaru suggestions here.

@lbenet
Copy link
Member Author

lbenet commented May 28, 2022

(Forgot to push the last commit...)

@lucaferranti
Copy link
Member

@lbenet I suggest we merge this and leave the pi vs -pi nuisance for a separate PR

@lucaferranti
Copy link
Member

also because since #533 is built on top on this and hence it also incorporates all the inf, sup changes, which makes review a little harder

@lbenet
Copy link
Member Author

lbenet commented May 29, 2022

I think the last commit includes the nuisances related to pi you mean.

I agree: let's merge this, and afterwards I'll rebase #533 to 1.0-dev

@lbenet lbenet merged commit cafc327 into 1.0-dev May 29, 2022
@lucaferranti lucaferranti deleted the lb/use_infsup branch May 29, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Planned for the major 1.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants