Skip to content

Remove generic AbstractArray fallbacks for position #26

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

Merged
merged 2 commits into from
Feb 9, 2025

Conversation

mtfishman
Copy link
Member

@mtfishman mtfishman commented Feb 7, 2025

Closes #16. This removes generic fallback definitions for the positions of eltype and ndims for AbstractArrays, which is safer since not all of them have eltype in the first position and ndims in the second position.

This was inspired by a bug I was hunting down while working on ITensor/QuantumOperatorDefinitions.jl#22, which was due to a BitArray using the generic position definition, which was incorrect because it doesn't have an eltype parameter since it is always Bool, and ndims is in the first position. In this PR, I added explicit correct definitions for BitArray.

@lkdvos @kmp5VT

I think this will be a good move. One issue this PR reminds me of is that right now default parameters are defined like:

default_type_parameters(::Type{<:Array}) = (Float64, 1)

i.e. they are defined in a list. This is ok, but I think it would be nicer if we could define them one at a time using the parameter names:

default_type_parameter(::Type{<:Array}, eltype) = Float64
default_type_parameter(::Type{<:Array}, ndims) = 1

I remember we tried that earlier on and it was tough to make type stable, but I think it would be good to revisit that (but not in this PR). (EDIT: Raised an issue here: #27.)

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.38%. Comparing base (61b969d) to head (6964e47).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/base/abstractarray.jl 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   84.86%   86.38%   +1.52%     
==========================================
  Files          10        9       -1     
  Lines         185      191       +6     
==========================================
+ Hits          157      165       +8     
+ Misses         28       26       -2     
Flag Coverage Δ
docs 47.87% <58.33%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Definitely agree to get rid of that definition, as this clearly might silently give you wrong results.

Also agreed to having the option of specifying defaults one at a time, in a future PR.

@kmp5VT
Copy link
Collaborator

kmp5VT commented Feb 7, 2025

This makes sense to me, I had concerns with these definitions before. Thanks Matt!

@mtfishman
Copy link
Member Author

@lkdvos any more comments?

Copy link
Contributor

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mtfishman mtfishman merged commit ca6b2c0 into main Feb 9, 2025
11 of 12 checks passed
@mtfishman mtfishman deleted the stricter_position branch February 9, 2025 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Decide on default position definitions for eltype and ndims
3 participants