Skip to content

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
@N5N3 N5N3 force-pushed the unified_constructer branch from c7c5585 to 90351f2 Compare March 22, 2022 14:48

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 force-pushed the unified_constructer branch from 294f2ef to c1fd9b3 Compare March 23, 2022 17:00
@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.

@N5N3 N5N3 force-pushed the unified_constructer branch from 690150f to 02c1449 Compare April 23, 2022 06:22
@oscardssmith
Copy link
Member

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 N5N3 force-pushed the unified_constructer branch from 02c1449 to bbd9b4b Compare May 6, 2022 10:38
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 N5N3 force-pushed the unified_constructer branch from bbd9b4b to a540037 Compare May 7, 2022 01:27
@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.

@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