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

Use default reader_kw=nothing to make FieldTimeSeries concretely typed #3898

Closed
glwagner opened this issue Nov 4, 2024 · 4 comments · Fixed by #3902
Closed

Use default reader_kw=nothing to make FieldTimeSeries concretely typed #3898

glwagner opened this issue Nov 4, 2024 · 4 comments · Fixed by #3902
Labels
performance 🏍️ So we can get the wrong answer even faster user interface/experience 💻

Comments

@glwagner
Copy link
Member

glwagner commented Nov 4, 2024

The default is not a concrete type which may cause performance issues when FieldTimeSeries is used as boundary condition or forcing in a kernel:

reader_kw = Dict{Symbol, Any}())

I suggest nothing as the default. Users can still change it if they want.

We could also use a NamedTuple instead of Dict.

@ali-ramadhan I think you maybe were not thinking that we use FieldTimeSeries in kernels and models now (not just for reading output)

@glwagner glwagner added user interface/experience 💻 performance 🏍️ So we can get the wrong answer even faster labels Nov 4, 2024
@ali-ramadhan
Copy link
Member

Ah good catch! Yeah nothing would be a good default. I can open a PR.

@glwagner
Copy link
Member Author

glwagner commented Nov 4, 2024

It could be nice to support both Dict and NamedTuple --- I see uses for both. Dict for post processing and NamedTuple for forcing models. Might already work is reader_kw is splatted in fact.

@ali-ramadhan
Copy link
Member

Hmmm, might have to default to NamedTuple as nothing cannot be splatted.

julia> f(x, y; kwargs...) = @show kwargs
f (generic function with 1 method)

julia> N = nothing

julia> f(1, 2; N...)
ERROR: MethodError: no method matching iterate(::Nothing)

Closest candidates are:
  iterate(::Base.AsyncGenerator, ::Base.AsyncGeneratorState)
   @ Base asyncmap.jl:362
  iterate(::Base.AsyncGenerator)
   @ Base asyncmap.jl:362
  iterate(::Core.MethodMatch, ::Int64)
   @ Base deprecated.jl:265
  ...

Stacktrace:
 [1] merge(a::@NamedTuple{}, itr::Nothing)
   @ Base ./namedtuple.jl:361
 [2] top-level scope
   @ REPL[12]:1

@glwagner
Copy link
Member Author

glwagner commented Nov 5, 2024

Hmmm, might have to default to NamedTuple as nothing cannot be splatted.

julia> f(x, y; kwargs...) = @show kwargs
f (generic function with 1 method)

julia> N = nothing

julia> f(1, 2; N...)
ERROR: MethodError: no method matching iterate(::Nothing)

Closest candidates are:
  iterate(::Base.AsyncGenerator, ::Base.AsyncGeneratorState)
   @ Base asyncmap.jl:362
  iterate(::Base.AsyncGenerator)
   @ Base asyncmap.jl:362
  iterate(::Core.MethodMatch, ::Int64)
   @ Base deprecated.jl:265
  ...

Stacktrace:
 [1] merge(a::@NamedTuple{}, itr::Nothing)
   @ Base ./namedtuple.jl:361
 [2] top-level scope
   @ REPL[12]:1

That works. I think they are sort of identical in the end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance 🏍️ So we can get the wrong answer even faster user interface/experience 💻
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants