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

Consistency in validity check during interval creation #468

Closed
Kolaru opened this issue May 31, 2021 · 2 comments
Closed

Consistency in validity check during interval creation #468

Kolaru opened this issue May 31, 2021 · 2 comments

Comments

@Kolaru
Copy link
Collaborator

Kolaru commented May 31, 2021

From a discussin started here: #461 (comment)

Currently decorated interval always check for validity (it is in the DecoratedInterval constructor) while for bare interval one need to use interval instead of the type constructor. It may be confusing for users and feel inconsistent.

One way around would be to have Interval always check by default and define a special unsafe_interval that doesn't perform any check and that we could use efficiently internally. Like that the types constructors would be safe for both bare and decorated intervals.

The internal constructors could look something like this:

struct Interval
    
    function Interval{T}(::Val{:unsafe}, a, b)
        # Create the interval without any validity check
    end
    
    function Interval{T}(a, b)
        # Check validity and so on
        return Interval{T}(Val{:unsafe}, a, b)
    end
end

unsafe_interval(a, b) = Interval{T}(Val{:unsafe}, a, b)
@lucaferranti
Copy link
Member

lucaferranti commented Jun 1, 2021

Hopefully these tables help in the discussion

  • Bare
Constructor checks? from string? rationalize?
Interval no (but can be enabled) no no
interval yes no no
.. yes no yes
@interval yes yes yes
  • Decorated
Constructor checks? from string? rationalize?
DecoratedInterval yes yes no
@decorated yes yes sometimes

For @decorated, the source code calls @interval with one input but only DecoratedInterval with two inputs, so that e.g. @decorated 0.1 would use rationalize under the hood but @decorated 0.1 0.3 would not, so at the moment we have


julia> a = @decorated 0.1
[0.0999999, 0.100001]_com

julia> b = @decorated 0.1 0.1
[0.1, 0.100001]_com

julia> big"0.1" ∈ a
true

julia> big"0.1" ∈ b
false

Currently, parse uses make_interval which uses atomic, hence it is not possible to construct an interval from a string without the "extra smartness" of rationalize, this leads to some ITF1788 tests failing in e.g. #464. I think there should also be a mechanism to construct an interval from a string without the extra rationalize step, i.e. something like interval(x::AbstractString) in the current constructors system (or Interval(x::AbstractString) in @Kolaru 's proposal, if I got their idea correctly). Practically, this would mean to remove make_interval from parse, or remove atomic from make_interval and call this extra step directly inside the macros (if we want the macro to do it).

@OlivierHnt
Copy link
Member

I believe this issue has been in resolved with #560.

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

No branches or pull requests

3 participants