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

Disallow simultaneously positive/negative InfExtendedReal #24

Merged
merged 3 commits into from
Aug 11, 2020

Conversation

omus
Copy link
Contributor

@omus omus commented Jul 29, 2020

Fixes #23

@codecov
Copy link

codecov bot commented Jul 29, 2020

Codecov Report

Merging #24 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #24   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines          175       175           
=========================================
  Hits           175       175           
Impacted Files Coverage Δ
src/common.jl 100.00% <100.00%> (ø)
src/infextendedreal/base.jl 100.00% <100.00%> (ø)
src/infextendedtime/base.jl 100.00% <100.00%> (ø)

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 90038fe...d43d9a8. Read the comment docs.

@omus omus force-pushed the cv/pos-neg-inf branch 2 times, most recently from 506a830 to 23bedda Compare July 29, 2020 13:49
@omus omus force-pushed the cv/pos-neg-inf branch from 23bedda to be9fa22 Compare July 29, 2020 14:07
@omus
Copy link
Contributor Author

omus commented Jul 29, 2020

Take note that some of these tests are not guaranteed to reproduce the problematic instance encountered in #23 due to requiring a specific value for the undefined finitevalue field. The tests currently in place are fairly likely to reproduce the problem but if we wanted to consistently reproduce the problem I'd need to make a subtype of real that only represented -Inf or Inf but not both. I'm not sure this is worth the effort.

@omus
Copy link
Contributor Author

omus commented Jul 30, 2020

On other thought on testing this. We could alternatively add an inner reinterpret method to directly set the bits associated with InfExtendedReal. This would be similar to how primitive types work:

julia> primitive type Demo 64 end

julia> Base.reinterpret(Demo, Int64(1))
Demo(0x0000000000000001)

@omus
Copy link
Contributor Author

omus commented Jul 30, 2020

With the latest change we now have guaranteed reproduction of the problematic instance.

@omus omus force-pushed the cv/pos-neg-inf branch from a280765 to d43d9a8 Compare July 31, 2020 02:00
@omus
Copy link
Contributor Author

omus commented Jul 31, 2020

Looks like I can't do new{T}(t...) on Julia 1.0 as we get this error:

LoadError: syntax: ... is not supported inside "new"

I've worked around this

@cjdoris cjdoris merged commit 9d4570c into cjdoris:master Aug 11, 2020
@omus omus deleted the cv/pos-neg-inf branch August 11, 2020 13:49
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.

InfExtendedReal can accidentally support a simultaneously positive/negative inf
2 participants