Drop all v1 deprecations for v2 by rofinn · Pull Request #214 · invenia/Intervals.jl

rofinn · 2023-04-07T01:21:20Z

Removes all deprecations placed in src/deprecations and fixes/deletes any relevant tests. Commits are split up by types of deprecations being dropped.

@omus I can't seem to set you as a reviewer, but I'm guessing you're the primary person who should review these changes?

codecov · 2023-04-07T01:36:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.85%. Comparing base (ba938f6) to head (eb2afc1).

Additional details and impacted files

@@            Coverage Diff             @@
##           v2-DEV     #214      +/-   ##
==========================================
+ Coverage   84.31%   93.85%   +9.54%     
==========================================
  Files          12       10       -2     
  Lines         848      700     -148     
==========================================
- Hits          715      657      -58     
+ Misses        133       43      -90

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

omus · 2023-05-24T21:04:31Z

I've allocated some bandwidth to review this tomorrow

omus · 2023-05-25T15:56:43Z

-    @testset "legacy deserialization" begin
-        # Serialized string generated on Intervals@1.2 with:
-        # `julia --project -E 'using Serialization, Intervals; sprint(serialize, AnchoredInterval{-1,Int}(2, true, false))'`.
-        buffer = IOBuffer(
-            SERIALIZED_HEADER *
-            "\x004\x10\x01\x10AnchoredInterval\x1f\v՞\x84\xec\xf7-`\x87\xbb" *
-            "S\xe1Á\x88A\xd8\x01\tIntervalsD\x02\0\0\x001\xff\xff\xff\xff\0\b\xe14\x10" *
-            "\x01\vInclusivity\x1f\v՞\x84\xec\xf7-`\x87\xbbS\xe1Á\x88A\xd8,\x02\0DML"
-        )
-
-        interval = deserialize(buffer)
-        @test interval isa AnchoredInterval
-        @test interval == AnchoredInterval{-1,Int,Closed,Open}(2)
-    end
-


You removed the legacy deserialization tests but didn't remove the code from compat.jl which is what this tests. I think the reason you removed this is that the removal of Inclusivity broke the code in compat.jl but we can make a minor change to continue supporting deserialization of the old Interval 1.2 types. It's definitely not necessary with a breaking version change but it's not hard to support:

# These `deserialize` methods are used to be able to deserialize intervals using the # structure that was used before Intervals 1.3. abstract type Inclusivity end function Serialization.deserialize(s::AbstractSerializer, ::Type{Inclusivity}) L = bound_type(deserialize(s)) R = bound_type(deserialize(s)) return L, R end function Serialization.deserialize(s::AbstractSerializer, ::Type{Interval{T}}) where T left = deserialize(s) right = deserialize(s) L, R = deserialize(s) # Deserialize `Inclusivity` return Interval{T,L,R}(left, right) end function Serialization.deserialize(s::AbstractSerializer, ::Type{AnchoredInterval{P,T}}) where {P,T} anchor = deserialize(s) L, R = deserialize(s) # Deserialize `Inclusivity` return AnchoredInterval{P,T,L,R}(anchor) end

With this updated compat.jl code these tests should pass.

Since this is already going into the break v2-DEV release I'm inclined to just drop the compat.jl code. Also, deserialization will likely break again with #217 anyway. If we don't drop it for v2 are we just going to retain this compat code indefinitely?

I agree we can't keep the serialization compat code forever but it is nice to have some compatibility with old serialized versions. I think having backwards compatible serialization with the latest previous version would be a good goal to have (1.9.0) in this case.

I'd personally probably include this compat change in this PR and then just remove it in #217 just to keep in the commit history.

omus · 2023-05-26T04:17:47Z


                # TODO: Sometimes expected_xor would get mutated in this call
-                @test symdiff([earlier], [later]) == expected_xor != union(expected_xor)
+                @test symdiff([earlier], [later]) == expected_xor == union(expected_xor)


This minor change is generated some major internal debate. In looking over this series of tests again the intent is to show that the IntervalSet behaviour works as expected and that the vector of Interval may or may not match. It appears that a commit you added (e122bfe) introduced the additional check to see what the resulting value actually is which makes the intent less clear.

In looking over that commit is seems you just cared about having a check to see what the value actually is so by following what I original intended and your intent we should just do:

Suggested change

@test symdiff([earlier], [later]) == expected_xor == union(expected_xor)

@test symdiff([earlier], [later]) == union(expected_xor)

resulting value actually is which makes the intent less clear.

Why is it less clear? The issue with the original check is that we were testing with multiple overloaded methods. If you were to read these tests without context you might be confused about why they don't match because our implementation of union for vectors of intervals is a bit weird. It seemed prudent to add a more explicit check for when we deprecated the union method.

I suppose they could have been separate checks, but I like that the change is all on one line. Anyway, I agree that going forward we don't need to check both anymore, since union works as expected now.

When we added the IntervalSet set methods tests we also added tests against vectors of intervals just to document any deviations in the behaviour but ultimately we didn't care what the result was.

This originally looked like:

@test symdiff([earlier], [later]) != union(expected_xor) @test symdiff(IntervalSet(earlier), IntervalSet(later)) == union(IntervalSet(expected_xor))

The vector of intervals test is just mirroring the IntervalSet test below but adjusts the comparison to the current behaviour

Anyway, I agree that going forward we don't need to check both anymore, since union works as expected now.

Sounds good.

omus · 2023-05-26T04:18:01Z

                @test setdiff(IntervalSet(later), IntervalSet(earlier)) == IntervalSet(expected_xor[2:2])

-                @test symdiff([earlier], [later]) == expected_xor != union(expected_xor)
+                @test symdiff([earlier], [later]) == expected_xor == union(expected_xor)


Suggested change

@test symdiff([earlier], [later]) == expected_xor == union(expected_xor)

@test symdiff([earlier], [later]) == union(expected_xor)

omus · 2023-05-26T04:23:33Z

            @test_throws MethodError symdiff(earlier, later)

-            # Using a vector of intervals as sets
-            @test union([earlier, later]) == [earlier, later]


These tests should stay as they serve to showcase why you need IntervalSet when dealing with sets of Intervals. The comment should be updated to say:

# Test using a vector of intervals as a set only for comparison purposes

Unfortunately, I'm unable to suggest these changes.

So you want to keep the test just to test the Base behaviour... and test the negation?

Correct. For the same reason we test vectors of intervals elsewhere. Unless we can stop end users from using vectors of intervals seeing weird behaviour it's good to document/record the behaviour of vectors of intervals with the Base set functions.

omus · 2023-05-26T04:25:40Z

-    @testset "legacy deserialization" begin
-        # Serialized string generated on Intervals@1.2 with:
-        # `julia --project -E 'using Serialization, Intervals; sprint(serialize, Interval(1, 2, true, false))'`.
-        buffer = IOBuffer(
-            SERIALIZED_HEADER *
-            "\x004\x10\x01\bInterval\x1f\v՞\x84\xec\xf7-`\x87\xbbS\xe1Á\x88A\xd8\x01\t" *
-            "IntervalsD\x01\0\0\0\0\b\xe0\xe14\x10\x01\vInclusivity\x1f\v՞\x84\xec\xf7" *
-            "-`\x87\xbbS\xe1Á\x88A\xd8,\x02\0DML"
-        )
-
-        interval = deserialize(buffer)
-        @test interval isa Interval
-        @test interval == Interval{Closed,Open}(1, 2)


Same as other deserialization comment

rofinn

Okay, I've reviewed and while I agree with most of the comments I'd like to confirm that we want to:

Do we want to maintain v1 deserialization in v2 (it'll be a lot of work/bloat for all the proposed breaking changes)?
Do we want to include tests of Base behaviour (ie: that union(intervalsets) != union(vectors))?

rofinn · 2023-05-29T18:20:51Z

-    @testset "legacy deserialization" begin
-        # Serialized string generated on Intervals@1.2 with:
-        # `julia --project -E 'using Serialization, Intervals; sprint(serialize, AnchoredInterval{-1,Int}(2, true, false))'`.
-        buffer = IOBuffer(
-            SERIALIZED_HEADER *
-            "\x004\x10\x01\x10AnchoredInterval\x1f\v՞\x84\xec\xf7-`\x87\xbb" *
-            "S\xe1Á\x88A\xd8\x01\tIntervalsD\x02\0\0\x001\xff\xff\xff\xff\0\b\xe14\x10" *
-            "\x01\vInclusivity\x1f\v՞\x84\xec\xf7-`\x87\xbbS\xe1Á\x88A\xd8,\x02\0DML"
-        )
-
-        interval = deserialize(buffer)
-        @test interval isa AnchoredInterval
-        @test interval == AnchoredInterval{-1,Int,Closed,Open}(2)
-    end
-


Since this is already going into the break v2-DEV release I'm inclined to just drop the compat.jl code. Also, deserialization will likely break again with #217 anyway. If we don't drop it for v2 are we just going to retain this compat code indefinitely?

rofinn · 2023-05-29T18:22:52Z

            @test_throws MethodError symdiff(earlier, later)

-            # Using a vector of intervals as sets
-            @test union([earlier, later]) == [earlier, later]


So you want to keep the test just to test the Base behaviour... and test the negation?

rofinn · 2023-05-29T18:41:21Z


                # TODO: Sometimes expected_xor would get mutated in this call
-                @test symdiff([earlier], [later]) == expected_xor != union(expected_xor)
+                @test symdiff([earlier], [later]) == expected_xor == union(expected_xor)


resulting value actually is which makes the intent less clear.

Why is it less clear? The issue with the original check is that we were testing with multiple overloaded methods. If you were to read these tests without context you might be confused about why they don't match because our implementation of union for vectors of intervals is a bit weird. It seemed prudent to add a more explicit check for when we deprecated the union method.

I suppose they could have been separate checks, but I like that the change is all on one line. Anyway, I agree that going forward we don't need to check both anymore, since union works as expected now.

omus · 2023-05-30T14:52:18Z

Do we want to maintain v1 deserialization in v2 (it'll be a lot of work/bloat for all the proposed breaking changes)?

I'd make an issue against the v2 milestone and deal with this before we make the official v2.0.0 release. I don't think we need to keep deserialization compatibility with Intervals 1.2 as that version is quite old but being able to load Intervals 1.9.0 would be nice to have in Intervals 2.0.0. I would 100% punt this work until the internal redesign is done though as otherwise it'll be annoying to update while the internal design is being iterated on.

Do we want to include tests of Base behaviour (ie: that union(intervalsets) != union(vectors))?

Yes, as long as we're testing other Base set functions in comparisons.jl we should should also include this test. We could remove the vector of intervals tests in which case we would also remove the test you mentioned. Maybe what we should so is define methods like Base.union(::AbstractVector{Interval} = throw(MethodError) to stop end users from mistakenly using these methods? In that case I'd be fine with removing the vector of intervals tests.

rofinn added 6 commits April 6, 2023 15:21

Drop deprecated DateTime constructors.

cde39c3

Drop deprecated Interval constructors.

066ac31

Drop deprecated Endpoint constructors.

8df18d1

Drop deprecated AnchoredInterval constructors.

02e5964

Drop deprecated Inclusivity type completely.

ed55f07

Drop interval set deprecations.

eb2afc1

rofinn self-assigned this Apr 7, 2023

rofinn changed the title ~~Rf/drop deps~~ Drop all v1 deprecations for v2 Apr 7, 2023

rofinn added this to the Intervals 2.0 milestone Apr 7, 2023

rofinn requested a review from omus April 12, 2023 01:12

rofinn mentioned this pull request May 18, 2023

[WIP] Interval bounds field #217

Open

4 tasks

omus requested changes May 26, 2023

View reviewed changes

rofinn commented May 29, 2023

View reviewed changes

	@test symdiff([earlier], [later]) == expected_xor == union(expected_xor)
	@test symdiff([earlier], [later]) == union(expected_xor)

Conversation

rofinn commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

omus commented May 24, 2023

Uh oh!

omus May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omus May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rofinn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omus commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rofinn commented Apr 7, 2023 •

edited

Loading

codecov Bot commented Apr 7, 2023 •

edited

Loading

omus May 25, 2023 •

edited

Loading

omus May 26, 2023 •

edited

Loading