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

NamedTuple Constructor Support for FieldArray Subtypes #25

Closed
wants to merge 1 commit into from
Closed

NamedTuple Constructor Support for FieldArray Subtypes #25

wants to merge 1 commit into from

Conversation

cadojo
Copy link

@cadojo cadojo commented Feb 18, 2024

I have found NamedTuple to be a performant way to initialize FieldArray subtypes. See, for example, the following code in AstrodynamicalModels.jl. I think it may also be useful sometimes to go in reverse: convert a FieldArray subtype back into a NamedTuple. Do you agree this may be useful, and without undesirable side effects? If so, should this PR include a Base.convert method too?

I got this idea from SciML's Base.NamedTuple method for LabelledArrays.jl.

Thanks for reading!

"""
A mutable vector, with labels, for 6DOF Cartesian states.
"""
Base.@kwdef mutable struct CartesianState{F} <: FieldVector{6,F}
    x::F = 0.0
    y::F = 0.0
    z::F = 0.0::F = 0.0::F = 0.0::F = 0.0

    CartesianState{F}(::UndefInitializer) where {F} = new{F}()
    CartesianState(::UndefInitializer) = CartesianState{Float64}(undef)

    CartesianState{F}(x, y, z, ẋ, ẏ, ż) where {F} = new{F}(x, y, z, ẋ, ẏ, ż)
    CartesianState(x, y, z, ẋ, ẏ, ż) = new{promote_type(typeof(x), typeof(y), typeof(z), typeof(ẋ), typeof(ẏ), typeof(ż))}(x, y, z, ẋ, ẏ, ż)
    CartesianState{F}(state::NamedTuple) where {F} =
        let
            (; x, y, z, ẋ, ẏ, ż) = merge((; x=zero(F), y=zero(F), z=zero(F), ẋ=zero(F), ẏ=zero(F), ż=zero(F)), state)
            CartesianState{F}(x, y, z, ẋ, ẏ, ż)
        end
    CartesianState(state::NamedTuple) = CartesianState{Float64}(state)
end

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (cc7d8fb) 96.87% compared to head (54b9e7a) 93.93%.

❗ Current head 54b9e7a differs from pull request most recent head fa5de02. Consider uploading reports for the commit fa5de02 to get more accurate results

Files Patch % Lines
src/StaticArraysCore.jl 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
- Coverage   96.87%   93.93%   -2.94%     
==========================================
  Files           1        1              
  Lines          64       66       +2     
==========================================
  Hits           62       62              
- Misses          2        4       +2     

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

@mateuszbaran
Copy link
Collaborator

Sounds reasonable to me, also for convert. Could you also add a test?

@mateuszbaran
Copy link
Collaborator

I'd also prefer to avoid unrelated formatting changes in this PR.

@cadojo
Copy link
Author

cadojo commented Feb 21, 2024

I'd also prefer to avoid unrelated formatting changes in this PR.

Yup, forgot to remove my editor's format-on-save option. I've reverted the formatting changes.

Could you also add a test?

I have added tests, but they fail due to missing Base methods for FieldArray types. I guess those must be supplied by StaticArrays? Does this PR belong in StaticArrays instead?

Test Failure Output
(StaticArraysCore) pkg> test
     Testing StaticArraysCore
      Status `/private/var/folders/vc/rjqggbq92mj_strb625fy2g40000gn/T/jl_FO9yCN/Project.toml`
  [1e83bf80] StaticArraysCore v1.4.2 `~/StaticArraysCore.jl`
  [8dfed614] Test
      Status `/private/var/folders/vc/rjqggbq92mj_strb625fy2g40000gn/T/jl_FO9yCN/Manifest.toml`
  [1e83bf80] StaticArraysCore v1.4.2 `~/StaticArraysCore.jl`
  [2a0f44e3] Base64
  [b77e0a4c] InteractiveUtils
  [56ddb016] Logging
  [d6f4376e] Markdown
  [9a3f8284] Random
  [ea8e919c] SHA v0.7.0
  [9e88b42a] Serialization
  [8dfed614] Test
Precompiling project...
  1 dependency successfully precompiled in 1 seconds. 1 already precompiled.
     Testing Running tests...
types: Error During Test at /Users/joey/StaticArraysCore.jl/test/runtests.jl:34
  Test threw exception
  Expression: __FieldArrayTest(1, 2, 3) isa SizedArray
  MethodError: no method matching size(::__FieldArrayTest)
  
  Closest candidates are:
    size(::AbstractArray{T, N}, ::Any) where {T, N}
     @ Base abstractarray.jl:42
    size(::Core.Compiler.StmtRange)
     @ Base show.jl:2774
    size(::BitVector)
     @ Base bitarray.jl:104
    ...
  
  Stacktrace:
    [1] length
      @ ./abstractarray.jl:315 [inlined]
    [2] isempty(a::__FieldArrayTest)
      @ Base ./abstractarray.jl:1220
    [3] typeinfo_prefix(io::IOContext{IOBuffer}, X::__FieldArrayTest)
      @ Base ./arrayshow.jl:585
    [4] show_vector(io::IOContext{IOBuffer}, v::__FieldArrayTest, opn::Char, cls::Char)
      @ Base ./arrayshow.jl:515
    [5] show(io::IOContext{IOBuffer}, X::__FieldArrayTest)
      @ Base ./arrayshow.jl:486
    [6] show_unquoted(io::IOContext{IOBuffer}, ex::__FieldArrayTest, ::Int64, ::Int64)
      @ Base ./show.jl:1443
    [7] show_unquoted(io::IOContext{IOBuffer}, ex::__FieldArrayTest, indent::Int64, prec::Int64, ::Int64)
      @ Base ./show.jl:1444
    [8] show_list(io::IOContext{IOBuffer}, items::Vector{Any}, sep::String, indent::Int64, prec::Int64, quote_level::Int64, enclose_operators::Bool, kw::Bool)
      @ Base ./show.jl:1688
    [9] show_list
      @ ./show.jl:1669 [inlined]
   [10] show_unquoted(io::IOContext{IOBuffer}, ex::Expr, indent::Int64, prec::Int64, quote_level::Int64)
      @ Base ./show.jl:2135
   [11] show_unquoted
      @ ./show.jl:1924 [inlined]
   [12] print(io::IOContext{IOBuffer}, ex::Expr)
      @ Base ./show.jl:1439
   [13] sprint(f::Function, args::Expr; context::Pair{Symbol, Bool}, sizehint::Int64)
      @ Base ./strings/io.jl:112
   [14] sprint
      @ ./strings/io.jl:107 [inlined]
   [15] eval_test(evaluated::Expr, quoted::Expr, source::LineNumberNode, negate::Bool)
      @ Test ~/.julia/juliaup/julia-1.10.1+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/Test/src/Test.jl:390
   [16] macro expansion
      @ ~/.julia/juliaup/julia-1.10.1+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/Test/src/Test.jl:669 [inlined]
   [17] macro expansion
      @ ~/StaticArraysCore.jl/test/runtests.jl:34 [inlined]
   [18] macro expansion
      @ ~/.julia/juliaup/julia-1.10.1+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
   [19] top-level scope
      @ ~/StaticArraysCore.jl/test/runtests.jl:4
types: Error During Test at /Users/joey/StaticArraysCore.jl/test/runtests.jl:35
  Test threw exception
  Expression: convert(NamedTuple, __FieldArrayTest(1, 2, 3)) isa NamedTuple
  MethodError: no method matching size(::__FieldArrayTest)
  
  Closest candidates are:
    size(::AbstractArray{T, N}, ::Any) where {T, N}
     @ Base abstractarray.jl:42
    size(::Core.Compiler.StmtRange)
     @ Base show.jl:2774
    size(::BitVector)
     @ Base bitarray.jl:104
    ...
  
  Stacktrace:
    [1] axes
      @ ./abstractarray.jl:98 [inlined]
    [2] collect(A::__FieldArrayTest)
      @ Base ./array.jl:761
    [3] _totuple(::Type{Tuple}, ::__FieldArrayTest)
      @ Base ./tuple.jl:425
    [4] Tuple(itr::__FieldArrayTest)
      @ Base ./tuple.jl:391
    [5] (NamedTuple{(:a, :b, :c)})(itr::__FieldArrayTest)
      @ Base ./namedtuple.jl:149
    [6] NamedTuple(array::__FieldArrayTest)
      @ StaticArraysCore ~/StaticArraysCore.jl/src/StaticArraysCore.jl:323
    [7] convert(::Type{NamedTuple}, array::__FieldArrayTest)
      @ StaticArraysCore ~/StaticArraysCore.jl/src/StaticArraysCore.jl:322
    [8] macro expansion
      @ ~/.julia/juliaup/julia-1.10.1+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/Test/src/Test.jl:669 [inlined]
    [9] macro expansion
      @ ~/StaticArraysCore.jl/test/runtests.jl:35 [inlined]
   [10] macro expansion
      @ ~/.julia/juliaup/julia-1.10.1+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
   [11] top-level scope
      @ ~/StaticArraysCore.jl/test/runtests.jl:4
Test Summary: | Pass  Error  Total  Time
types         |   19      2     21  1.0s
ERROR: LoadError: Some tests did not pass: 19 passed, 0 failed, 2 errored, 0 broken.
in expression starting at /Users/joey/StaticArraysCore.jl/test/runtests.jl:3
ERROR: Package StaticArraysCore errored during testing

@mateuszbaran
Copy link
Collaborator

Yup, forgot to remove my editor's format-on-save option. I've reverted the formatting changes.

Thanks!

I have added tests, but they fail due to missing Base methods for FieldArray types. I guess those must be supplied by StaticArrays? Does this PR belong in StaticArrays instead?

I've checked and this indeed needs some methods from StaticArrays.jl. I think it would be much easier to add your methods to StaticArrays.jl.

@cadojo
Copy link
Author

cadojo commented Feb 22, 2024

Closing in favor of JuliaArrays/StaticArrays.jl#1246. Thanks!

@cadojo cadojo closed this Feb 22, 2024
@cadojo cadojo deleted the named-tuple-constructor branch February 22, 2024 01:13
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