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

Make Core.checked_dims public #54327

Open
LilithHafner opened this issue May 1, 2024 · 6 comments
Open

Make Core.checked_dims public #54327

LilithHafner opened this issue May 1, 2024 · 6 comments
Labels
domain:arrays [a, r, r, a, y, s] kind:feature Indicates new feature / enhancement requests

Comments

@LilithHafner
Copy link
Member

It turns out it's nontrivial to implement Core.checked_dims (e.g. #54255). I want it to be public from base so folks don't have to re-implement it in packages.

@KristofferC
Copy link
Sponsor Member

I want it to be public from base so folks don't have to re-implement it in packages.

Any examples of such implementations that folks had to reimplement?

@giordano
Copy link
Contributor

giordano commented May 2, 2024

Any examples of such implementations that folks had to reimplement?

https://github.com/JuliaArrays/FixedSizeArrays.jl/blob/e1c3b345e60492ef057af3578d0f7a8a6ad99541/src/FixedSizeArrays.jl#L56-L82

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 2, 2024

It also turned out to be occasionally useful just inside base, if you grep a bit (it is roughly equivalent to reduce(checked_mul, dims), but with special handling of zeros and typemax to correct some edge cases of those), so I think it is reasonable to export. I know FSA decided to implement it differently for themselves, though that will be necessary for packages that want to support older versions anyways.

@nsajko
Copy link
Contributor

nsajko commented May 3, 2024

IMO it would be a bit weird to export Core.checked_dims, given that it's not generic and it's implementation is necessarily clumsy due to bootstrapping concerns.

FTR the function in FSA also supports just Int, but it would be easy to make it generic.

@nsajko nsajko added domain:arrays [a, r, r, a, y, s] kind:feature Indicates new feature / enhancement requests labels May 3, 2024
@LilithHafner
Copy link
Member Author

LilithHafner commented May 3, 2024

Any examples of such implementations that folks had to reimplement?

https://github.com/LilithHafner/MallocArrays.jl/blob/0b6dbdc012e1058b2b64d0f94863eff4120def85/src/MallocArrays.jl#L10-L27


Yes, we'd need to make it a bit more generic and move it to Base. Possibly by defining Base.checked_dims(x...) = Core.checked_dims(Int.(x)...). Supporting Int only is okay/good b.c. that's what dims are represented/retrieved as, but auto-conversion is also reasonable.

@nsajko
Copy link
Contributor

nsajko commented May 3, 2024

Supporting Int only is okay/good b.c. that's what dims are represented

This is not a rule, e.g.:

julia> map(BigInt, 1:7)
1:7

julia> typeof(size(ans))
Tuple{BigInt}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] kind:feature Indicates new feature / enhancement requests
Projects
None yet
Development

No branches or pull requests

5 participants