IntervalSets.jl Add copy

This PR adds a copy method. The easiest way to do this was to call deepcopy, @ararslan @timholy is there any hidden issue in doing this?

Sep 12 '19 08:09 dlfivefifty

FWIW, I remember Jeff being opposed to having a copy for immutable objects, and I think that Base has removed a number of copy methods already. Eg.

julia> copy((1,2))
ERROR: MethodError: no method matching copy(::Tuple{Int64,Int64})

Sep 12 '19 13:09 cstjean

I think this is a better example:

julia> copy(1:2)
1:2

Sep 12 '19 15:09 dlfivefifty

is there any hidden issue in doing this?

With using deepcopy, you mean? Not as far as I'm aware. It ensures that BigFloats are properly copied because (IIRC) copy(::BigFloat) is a no-op as it is with the immutable number types, and deepcopy itself is a no-op for bits types.

Sep 12 '19 16:09 ararslan

Ah I didn't realise BigFloat behaved like that:

julia> copy(x) === x
true

julia> BigFloat(1) === BigFloat(1)
false

Perhaps I should just do a standard copy explicitly for Interval.

Sep 12 '19 16:09 dlfivefifty

If you want to make sure that mutating a BigFloat in one interval doesn't affect another interval, deepcopy is probably your best bet.

Sep 12 '19 16:09 ararslan

BigFloats are mutable? In that case isn't its copy behaviour a bug?

Sep 12 '19 16:09 dlfivefifty

¯\_(ツ)_/¯

Sep 12 '19 16:09 ararslan

BigFloat is supposed to be semantically immutable but their implementation doesn't let us do that now. copy(1:2) has to work because it's part of the AbstractArray interface. Given that copy(3) works then I think it's good to define it for intervals too.

deepcopy can be a huge performance trap, so I'm a bit opposed to implementing copy via deepcopy. What problem did using deepcopy solve?

Sep 14 '19 08:09 timholy

I don’t think it actually solves a problem. I’m inclined to change it to

copy(d::Domain) = d

Sep 14 '19 10:09 dlfivefifty

Isn't defining copy for concrete subtypes like Interval much safer option? If it's too cumbersome, how about

copy(d::Domain) = isbits(d) ? d : deepcopy(d)

Sep 14 '19 22:09 tkf

You are certainly right, at least in theory. In practice, I can’t think of a case where it matters.

But for the sake of not intentionally writing “bad“ code I’ll make that change and only override for TypedEndpointsInterval.

Sep 14 '19 22:09 dlfivefifty