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

Preserve more FieldArrays with parametric eltype. #1064

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

Conversation

N5N3
Copy link
Contributor

@N5N3 N5N3 commented Aug 1, 2022

Before this PR, user needs to define similar_type for their own FieldArrays with parametric eltype.
After this PR, these FieldArrays would be preserved automatically if there's no Size change. (Inspired by JuliaLang/julia#46213)

This PR also change the default similar type for mutable FieldArray to MArray.

@N5N3 N5N3 force-pushed the similar-type branch 2 times, most recently from c6e2676 to afc9759 Compare August 1, 2022 13:22
And return a `MArray` for mutable `FieldArray`
@vtjnash
Copy link

vtjnash commented Aug 1, 2022

This code is likely wrong, as mentioned in the manual for the purpose of this function:
https://docs.julialang.org/en/v1/manual/methods/#Building-a-similar-type-with-a-different-type-parameter
and in the issue mentioning why we don't currently provide this method:
JuliaLang/julia#35543 (comment)

Comment on lines 131 to 134
# FieldArrays with parametric `eltype` would be adapted to the new `eltype` automatically.
A′ = Base.typeintersect(base_type(A), StaticArray{Tuple{Tuple(S)...},T,length(S)})
# But extra parameters are disallowed here. Also we check `fieldtypes` to make sure the result is valid.
isconcretetype(A′) && fieldtypes(A′) === ntuple(Returns(T), Val(prod(S))) && return A′
Copy link
Contributor Author

@N5N3 N5N3 Aug 2, 2022

Choose a reason for hiding this comment

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

I still think this branch is useful for FieldArrays with only one parameter for eltype.
Check in L134 seems sufficient for me, as a concrete type with matched fieldtypes should be valid to return.

The `typeintersect` must return a concrete type now.
And it's `fieldtypes` should match our doc example.
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.

2 participants