Make StepRange safe for more number types

[timholy]

Currently StepRange is the only range type in Base that's potentially usable for objects where x+one(x) may not be definable. This relaxes what I suspect to be an unnecessary constraint---wraparound only seems relevant for Integer types.

[timholy]

CC @quinnj for the changes to the Dates tests. OK?

[ajkeller34]

I very much appreciate your efforts with this. I'm so glad to see these issues receiving attention.

Might I suggest oftype(stop-start, 1) instead of one(stop-start)? This way, there doesn't need to be a distinction made for integers. In Unitful.jl I would eventually like integers with units to be a subtype of Integer rather than Real (actually Signed or Unsigned would be even better), so that numbers with units retain the most specific abstract parent.

Edit: okay maybe Signed or Unsigned is a bit much from the perspective of what numbers should have units, but my point is that it'd be great if numbers with units key in naturally to existing abstract types for numbers.

[timholy]

If something is an Integer, then shouldn't one(x) be of the same type as x? To me that almost seems like a defining trait of Integer, that they are generated by addition of one.

I'm also uneasy with the notion of convert(Meter, 1) working at all. (Meter(1) is a constructor call, so I'm fine with that.)

[ajkeller34]

They are generated by the additive group generator. Let's not confuse that with multiplicative identity, even when it happens to work for integers without units. I 100% endorse the proposal for an additive group generator by @StefanKarpinski in #16116. If I had that function to play with, units would work so much better.

[ajkeller34]

I came to the conclusion earlier that so long as promotion doesn't automatically strip or add units, I'm okay with convert behaving as a unit stripper/adder (although in my package, the unit and numeric types are both specified, so you won't ever see Meter like that). If someone is explicitly demanding a number now have units, I think that's acceptable. We just want to avoid implicit conversion which can lead to errors and confusion.

To be clear, there is another syntax in Unitful which is more questionable, convert(km, 1m). I should likely not be abusing convert this way, since here km is an object, not a type, indicating I want a unit conversion that entails changing the numeric value, rather than a type conversion that adds/removes units. Maybe I should make my own unitconvert function.

To summarize, km is an object in the following:

convert(typeof(1km), 1) gives 1 km
convert(typeof(1), 1km) gives 1
convert(km, 1m) gives 1//1000 km
convert(km, 1) fails

I'm quite happy with the first two but would understand objections to the last two.

[timholy]

julia> convert(typeof(1km), 1s)
ERROR: Dimensional mismatch.
 in error(::String) at ./error.jl:21
 in convert(...) at /home/tim/.julia/v0.5/Unitful/src/Unitful.jl:926
 in convert(...) at /home/tim/.julia/v0.5/Unitful/src/Unitful.jl:1004
 in eval(::Module, ::Any) at ./boot.jl:234
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##3#4{Base.REPL.REPLBackend})() at ./event.jl:46

julia> convert(typeof(1km), convert(typeof(1), 1s))
1 km

So it works differently if you first convert to some other intermediary. I can imagine similar behavior emerging by accident (e.g., if YCbCr->HSV fails because I forgot to define it, but YCbCr->RGB->HSV works), but doing it by design doesn't make sense to me.

[StefanKarpinski]

I have to say that using convert to go from unitless to unitful and vice versa strikes me as iffy.

[ajkeller34]

I hope we can isolate discussion about convert from what we should do with one(x). If people don't like either usage of convert I'll try to get rid of that in my package, but please raise that as an issue in Unitful and don't let that detract from my legitimate concerns about how one(x) is used in base.

[timholy]

No arguments here that one is problematic and needs to be fixed. It's such an important point, I even put a 0.6.0 milestone on it this morning 😃 .

[timholy]

Our poor CI seems to be having a rough day. AFAICT none of those failures are relevant.

[timholy]

Since we're in feature-freeze, I'll let someone else decide about merging this.

[omus]

Wouldn't this be a more general way of handling overflow issues without using one or zero?

function steprange_last_empty(start, step, stop)
    prev = start - step
    last = prev < start ? prev : start + step
    return last
end

[timholy]

As far as the implementation of this function goes, if you get here, you already know that step is in the opposite direction of zero as that between start and stop---see line 33 above. So if you make use of step, I don't think you need the branch. But please check!

Or are you proposing this logic as a more general way to detect that the range is empty, without needing even zero?

[omus]

You are correct that you don't need the extra branch. May be worth adding a comment to the function.

Or are you proposing this logic as a more general way to detect that the range is empty, without needing even zero?

I think the code using zero is cleaner and probably more performant.

[quinnj]

Changes to the tests look fine by me.

[timholy]

Has it been decided that this isn't 0.5 material? I think you can basically regard this as a bugfix, for when one isn't available.

[tkelman]

This seems reasonable and hopefully low-risk to me, but I'd want to keep an eye on packageevaluator just in case (it's having some freezing trouble due to unreliable downloads that I'm working on fixing right now, so hasn't run to completion in a couple days).

[timholy]

Thanks. How about we merge once you've got packageevaluator working again.

[tkelman]

curlrc to the rescue, the currently executing run looks like it should finish properly.

[tkelman]

instead of one, should these stay equality tests but use pos_step ?

[timholy]

I'd argue that the specific choices in how an empty range is implemented is an internal detail that we actively want to avoid testing.

[timholy]

Good to go here?