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

Float-based ranges #9339

Closed
jtanderson opened this issue May 22, 2020 · 5 comments · Fixed by #10209
Closed

Float-based ranges #9339

jtanderson opened this issue May 22, 2020 · 5 comments · Fixed by #10209

Comments

@jtanderson
Copy link

jtanderson commented May 22, 2020

Proposal: It would be help in many situations to make the Range(Float64, Float64) and Range(Float32, Float32) types enumerable with a step size (default to 1).

One potential pain point is the constructor requiring three arguments instead of 2 now so the X..Y syntax won't work if one wants to specify the delta.

Looking at the code in "range.cr" I think a simple way to get at the enumeration is to put an internal function to the range that does a responds_to?(:succ) on the current range element to either use the provided method or to try and infer it by doing something like current + step_size. A slight downside of this is that now the underlying type needs an operator+ defined.

@straight-shoota
Copy link
Member

The use case is almost identical to #9327 and the solutions discussed there (e.g. #9327 (comment)) should work for any non-discrete type, including floats.
So the general idea would be to not encode the step size directly into the Range, but have it as an option when iterating the range. That's more flexible and doesn't even do much changes to the API.

@jtanderson
Copy link
Author

Agreed, the #step support mentioned there would work well for iteration.

@straight-shoota
Copy link
Member

Range#step only works for types that define #succ. Types like Float or Time have non-discrete values without a clear successor.

Range#step could use addition instead. The semantics of the by argument changes from "the number of successors between steps" to "delta between steps". This adds a lot more flexibility to the step method.
For Int and Char both meanings are equivalent and using addition instead would actually improve performance for them.

The only other type in stdlib that implements #succ is String. That won't work with addition unless we added String#+(Int) for that purpose. But this method wouldn't be very practical and we'd probably hide it away. That could probably work. Steps with String is kind of an outlier for a very specific use case. But, there might be similar use cases for other types, so I'm hesitant about special treatment.

So instead we could create a proper interface for that. Maybe #succ could take an argument and Range#step would just pass by to that method. Then it's up to the type to interpret the meaning of it.

But actually, this method already exists and even shares the name and intention: Number#step. It works with any number type, including Float.

So there is a very easy solution: Range#step should simply delegate to begin.step(to: end, by: by). Then it's up to the value type to implement the behaviour.

@asterite
Copy link
Member

@straight-shoota brilliant!

@straight-shoota
Copy link
Member

Number#step needed some fixup (#10130) but once that's done, we can proceed with adding delegation from Range#step.

Number#spec also needs an exclusive argument and that's an entirely useful addition on its own.

Range#step can delegate optionally if begin responds to #step. Otherwise the current implementation based on #succ can stay. #succ is also used by Range#each. Maybe that could later be switched to delegate to #step. But it only work for types with discrete values, or otherwise a step size (=by) needs to be provided.

The implementation of Number#step is pretty generic and works for any type that defines meaningful #+, #- and #<=> (= Comparable) methods. That includes Time and Time::Span (#9327).

It seems like a good idea to move the implementation out of Number and make it reusable for other types.
So, should that be a module Stepable? Or because it's tightly related to Comparable maybe it should be a part of that, like module Comparable::Step?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants