refactor generic hybrid#274
Conversation
📚 Documentation preview 🚀Preview URL: https://EarthyScience.github.io/EasyHybrid.jl/previews/PR274/ Note The preview will be available once the documentation build completes successfully, and will reflect the last successful build for this PR. |
There was a problem hiding this comment.
Code Review
This pull request consolidates SingleNNHybridModel and MultiNNHybridModel into a unified HybridModel struct, simplifying model construction, initialization, and forward pass logic across the codebase. The review feedback highlights several critical performance and correctness improvements: resolving a recursive configuration nesting issue in tune.jl by excluding the :config field before merging, avoiding inefficient vector comprehensions when constructing NamedTuples in prepare_data.jl, and replacing iterative merge loops with type-stable map operations in GenericHybridModel.jl to prevent excessive allocations and type instability during parameter initialization and the forward pass.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Let's really stick with |
well, with the proposed changes, it actually does that when the arguments are the appropriate ones. This is handle in the forward run definition of function (m::HybridModel)(ds_k::Tuple, ps, st) ... endI will bring-back the interface The recipe is basically,
struct test
a
b
end
function test(args...)
a, b = args
return test(a,b)
end
function (m::test)(data, ps, st)
# your logic using `m` and `data`
return out, st_new
end |
|
But the users never does a forward run with this signature (?), it is under the hood: The constructor is clear but the user then runs a function with the name of his hybrid model? Or can you give any example for a forward run with |
|
updated all function constructors to @BernhardAhrens good to merge now? |
|
Nice! I like the new tutorial @lazarusA Ready to be merged |
BernhardAhrens
left a comment
There was a problem hiding this comment.
Thanks :) Nice refactor - should make things easier down the road
With this PR all the following will be removed and unified into only one
HybridModelstruct/function.and now only
HybridModel is needed.
@BernhardAhrens this will be highly breaking for the user.
Edit:
this works again
constructHybridModel!closes #191