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

add default constructors for AbstractRange #48894

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

putianyi889
Copy link
Contributor

@putianyi889 putianyi889 commented Mar 4, 2023

Copy link
Member

@inkydragon inkydragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to add constructors for abstract types?

julia> methods(AbstractRange)
# 0 methods for type constructor

julia> supertype(AbstractRange)
AbstractVector (alias for AbstractArray{T, 1} where T)

julia>  methods(AbstractVector)
# 0 methods for type constructor

julia> supertype(AbstractVector)
Any

@inkydragon inkydragon added the ranges Everything AbstractRange label Mar 10, 2023
@putianyi889
Copy link
Contributor Author

As you can see from added tests, it does eltype conversion for general ranges.
AbstractArray has constructors.

putianyi889 added a commit to putianyi889/PTYQoL.jl that referenced this pull request Nov 5, 2023
@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Mar 21, 2024
@jishnub
Copy link
Contributor

jishnub commented Mar 22, 2024

@putianyi889 This seems like a good idea. Is this ready from your side?
We would probably want more tests.

@putianyi889
Copy link
Contributor Author

Is this ready from your side?

confirm.

We would probably want more tests.

not sure how many. maybe add 2-3 segments to every conversion test?

base/range.jl Outdated Show resolved Hide resolved
test/ranges.jl Outdated Show resolved Hide resolved
putianyi889 and others added 2 commits March 25, 2024 09:52
Co-authored-by: Sheehan Olver <solver@mac.com>
base/range.jl Outdated Show resolved Hide resolved
@jishnub
Copy link
Contributor

jishnub commented Aug 17, 2024

Could you add tests for other range types, such as StepRangeLen and StepRange, including cases where the elements are floating-point types?

Comment on lines +278 to +280
AbstractRange{T}(r::AbstractRange) where T = T(first(r)):T(step(r)):T(last(r))
AbstractArray{T,1}(r::AbstractRange) where T = AbstractRange{T}(r)
AbstractArray{T}(r::AbstractRange) where T = AbstractRange{T}(r)
Copy link
Contributor

@mcabbott mcabbott Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some failures from this T(first(r)):T(step(r)):T(last(r)) construction:

julia> convert(AbstractArray{Float32}, 0 * range(1, 2, 10)) |> length
ERROR: ArgumentError: range step cannot be zero

julia> convert(AbstractArray{Float16}, range(1/43^2, 1, 43)) |> length
42

I assume that range(start=T(first(r)), stop=T(last(r)), length=length(r)) would avoid these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out!

range would fix StepRangeLen, but changes OrdinalRange to StepRangeLen as well. Anyway, either OrdinalRange or StepRangeLen needs a specialized method.

As for the general fallback, I would keep the colon syntax until there are other issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait the priority surely is to get the right answer?

If necessary, there can be specialised paths which try also to preserve certain types, like UnitRange and StepRange and LinRange, when possible. But the default path should not rely on a:b:c colon magic to keep away from factually wrong answers.

putianyi889 added a commit to putianyi889/PTYQoL.jl that referenced this pull request Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forget me not PRs that one wants to make sure aren't forgotten ranges Everything AbstractRange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants