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

Code clean and reuse for constructor. #1016

Merged
merged 11 commits into from
May 16, 2022

Conversation

N5N3
Copy link
Contributor

@N5N3 N5N3 commented Mar 22, 2022

Seperated from #1009.

This PR aims to unify the implement of StaticArray's constructor with a new method construct_type.
It returns a concrete constructor based on src <: Union{Tupe, StaticArray, AbstractArray} and the target type SA (if possible).
e.g.

julia> StaticArrays.construct_type(SMatrix{2},(1,2,3,4))
SMatrix{2, 2, Int64} (alias for SArray{Tuple{2, 2}, Int64, 2})

julia> StaticArrays.construct_type(SMatrix{2},(1.,2,3,4))
SMatrix{2, 2, Float64} (alias for SArray{Tuple{2, 2}, Float64, 2})

julia> StaticArrays.construct_type(SMatrix,(1.,2,3,4)) # the output's size is not static.
ERROR: DimensionMismatch: No precise constructor for SMatrix found. Length of input was 4.

Thus, we only need to mannually define the constructor with precise size, eltype and ndims.
Unneeded dispatch has been removed.
Edit1: Since 3rd party StaticArray might prefer previous design, the default construct_type returns the input SA directly to keep compatibility.
Edit2: construct_type for FieldArray has been implemented. Now FieldArray's default constructor support eltype promotion.

julia> struct Point2D{T} <: FieldVector{2,T}
           x::T
           y::T
       end

julia> Point2D(1,1.0)
2-element Point2D{Float64} with indices SOneTo(2):
 1.0
 1.0

This PR also restrict the input wrapping within constructor.
If multiple arguments are provided, all of them should be treated as the result's elements, and re-wrapping is not allowed anymore.
For example

julia> Scalar(1, 2)
ERROR: DimensionMismatch: No precise constructor for Scalar found. Length of input was 2.
julia> Scalar((1, 2))
Scalar{Tuple{Int64, Int64}}(((1, 2),))
julia> SVector{1}(1, 2)
ERROR: DimensionMismatch: No precise constructor for SVector{1} found. Length of input was 2.
julia> SVector{1}((1, 2))
1-element SVector{1, Tuple{Int64, Int64}} with indices SOneTo(1):
 (1, 2)

Close #809.
Close #911. (fix constructor missing)

@N5N3 N5N3 changed the title clean. Code clean and reuse for constructor. Mar 22, 2022
@@ -1,7 +1,7 @@
# Allow no new ambiguities (see #18), unless you fix some old ones first!

const allowable_ambiguities = VERSION ≥ v"1.7" ? 60 :
VERSION ≥ v"1.6" ? 61 : error("version must be ≥1.6")
const allowable_ambiguities = VERSION ≥ v"1.7" ? 10 :
Copy link
Contributor

Choose a reason for hiding this comment

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

🎆

@N5N3 N5N3 force-pushed the unified_constructer branch 3 times, most recently from c8eaf3b to 294f2ef Compare March 23, 2022 16:52
@N5N3 N5N3 closed this Mar 23, 2022
@N5N3 N5N3 reopened this Mar 23, 2022
@N5N3 N5N3 marked this pull request as draft March 24, 2022 06:01
@N5N3 N5N3 force-pushed the unified_constructer branch 2 times, most recently from 71cd505 to aee1ace Compare March 25, 2022 14:50
@N5N3 N5N3 marked this pull request as ready for review March 25, 2022 15:03
@N5N3 N5N3 force-pushed the unified_constructer branch 5 times, most recently from 17e8b43 to 2fd8abb Compare March 26, 2022 11:02
Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

Most of the changes seem great! I have added some comments.

src/convert.jl Outdated Show resolved Hide resolved
src/SizedArray.jl Show resolved Hide resolved
src/SizedArray.jl Show resolved Hide resolved
src/convert.jl Show resolved Hide resolved
src/convert.jl Outdated Show resolved Hide resolved
src/convert.jl Outdated Show resolved Hide resolved
test/SizedArray.jl Show resolved Hide resolved
test/SizedArray.jl Outdated Show resolved Hide resolved
@oscardssmith
Copy link

Is this ready to merge?

N5N3 added 4 commits May 6, 2022 18:37
Similar to `similar_type`, but not fallback to `SArray`. It is used to pick the most `concrete` constructor with `size`, `eltype` and `ndims` defined.
And fix for input with non-1 based axes.
N5N3 added 2 commits May 7, 2022 09:20
With `construct_type`, there's no need to keep all these dispatches.
This also fix empty construction for `S/MVector`, and remove most of the ambiguities.
N5N3 added 4 commits May 7, 2022 09:20
1. remove `StaticSquareMatrix` (`StaticMatrix{N,N}` should be shorter and clear enough)
2. Add missing Test.
And convert comments to docstring.
@N5N3
Copy link
Contributor Author

N5N3 commented May 7, 2022

I've added a warning to construct_type's docstring that invalid overload of this function might cause infinite recursion.
The main concern might be the influence on downside packages.
The default behavior should be exactly the same, but I only test this PR with Rotations.jl, Geodesy.jl.

src/util.jl Outdated Show resolved Hide resolved
@hyrodium
Copy link
Collaborator

Sorry for the late review. The rest of the code looks great to me.

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.

MMatrix generation with @MMatrix requires equal type for elements Scalar(1, 2) === Scalar((1, 2))?
4 participants