Simplify Reaction creation (#9)

* Simplify Reaction creation

Use InteractiveUtils.subtypes(AbstractReaction) to find all subtypes of
AbstractReaction, instead of a custom reaction_factories Dict.

This should be simpler and more robust, as removes risk of getting
out of sync with Julia's type system.

Reactions can implement
create_reaction(ReactionType::Type{<:AbstractReaction}, base::ReactionBase)
if necessary to set fields etc.

Module __init__() functions with calls to
PB.add_reaction_factory()
can now all be removed

* More robust strategy if duplicate Reaction names found

Now log warning, and remove all duplicates to force error if a model
attempts to use a duplicated name.

* Better fix for duplicate Reactions - @warn for each duplicate

Project.toml

@@ -1,7 +1,7 @@
 name = "PALEOboxes"
 uuid = "804b410e-d900-4b2a-9ecd-f5a06d4c1fd4"
 authors = ["Stuart Daines <[email protected]>"]
-version = "0.15.0"
+version = "0.16.0"
 
 [deps]
 BenchmarkTools = "6e4b80f9-dd63-53aa-95a3-0cdb28fa8baf"

docs/src/Reaction API.md

@@ -29,9 +29,7 @@ setvalue!
 
 ## Registering with PALEOboxes framework
 ```@docs
-reaction_factories
-add_reaction_factory
-reaction_factory
+create_reaction(ReactionType::Type{<:AbstractReaction}, base::ReactionBase)
 ```
 
 ## Optional initialisation callbacks to define Domain Grids and array sizes

src/Domain.jl

@@ -277,7 +277,7 @@ end
 
 
 function create_domain_from_config(
-    name::AbstractString, domainID::Integer, conf_domain::Dict{Any,Any}, external_parameters::Dict{String, Any}
+    name::AbstractString, domainID::Integer, conf_domain::Dict{Any,Any}, external_parameters::Dict{String, Any}, rdict::Dict{String, Type}
 )
 
     for k in keys(conf_domain)
@@ -317,7 +317,7 @@ function create_domain_from_config(
                 push!(
                     domain.reactions, 
                     create_reaction_from_config(
-                        domain, reactname, conf_reaction, classname, domain.parameters
+                        classname, rdict, domain, reactname, conf_reaction, domain.parameters
                     )
                 )
             else

src/Model.jl

@@ -256,6 +256,8 @@ function create_model_from_config(
     conf_domains = conf_model["domains"]
 
     # create domains
+    rdict = find_all_reactions()
+    @info "generated Reaction catalog with $(length(rdict)) Reactions"
     for (name, conf_domain) in conf_domains
         nextDomainID = length(model.domains) + 1
         @info "creating domain '$(name)' ID=$nextDomainID" 
@@ -264,7 +266,7 @@ function create_model_from_config(
         end       
         push!(
             model.domains, 
-            create_domain_from_config(name, nextDomainID, conf_domain, model.parameters)
+            create_domain_from_config(name, nextDomainID, conf_domain, model.parameters, rdict)
         )     
     end
 

src/Reaction.jl

@@ -336,14 +336,15 @@ end
 "Create new ReactionXXXX of specified classname from config file, set parameters,
 read variable mapping and initial values (to be set later by _configure_variables!)"
 function create_reaction_from_config(
+    classname::AbstractString,
+    rdict::Dict{String, Type},
     domain::Domain,
     name,
     conf_reaction,
-    classname,
     external_parameters::Dict{String, Any}
 )
 
-    newreaction = _create_reaction(classname, name, external_parameters)
+    newreaction = create_reaction(rdict, classname, name, external_parameters)
     newreaction.base.domain = domain
 
     for k in keys(conf_reaction)

src/ReactionFactory.jl

@@ -1,101 +1,57 @@
 import Printf
+import InteractiveUtils
 
 """
-    const reaction_factories = Dict{classname::String, Function}()
+    find_all_reactions() -> Dict{String, Type}
 
-Dictionary of reaction factories.
+Use `InteractiveUtils.subtypes(AbstractReaction)` to find all currently loaded subtypes off
+AbstractReaction, and create a `Dict` with last part of the name of the Type
+as key (ie without the module prefix) and Type as value.
 
-A module containing Reactions should define a module `__init__()` function that includes a call
-to [`add_reaction_factory`] for each Reaction. This registers functions that then return instances of the Reaction structs.
-
-    "Install create_reactionXXX factories when module imported"
-    function __init__()
-        PB.add_reaction_factory(ReactionReservoirScalar)
-    end
-
-"""
-const reaction_factories = Dict{String, Function}()
-
-"""
-    reaction_factory(ReactionType::Type{<:AbstractReaction}; [, operatorID=[1]] [,extra_par_fields], [, set_pars::Tuple]) -> Function
-
-Return a function that will create Reaction of type `rt` when called.
-    
-Optionally, after creation, set and freeze Parameters in `set_pars` (a Tuple of Pairs of name=>value eg ("total"=>false,) )
+Any Types that generate non-unique keys (eg Module1.MyReactionType and Module2.MyReactionType) will generate
+a warning, and no entry will be added to the Dict (so if this Reaction is present in a config file, it will
+not be found and will error).
 """
-function reaction_factory(
-    ReactionType::Type{<:AbstractReaction}; 
-    operatorID=[1],
-    extra_par_fields::Vector{Symbol}=Symbol[], # eg [:pars_stoich]
-    set_pars::Tuple{Vararg{Pair}}=()
-)
-
-    function create_init_reaction(base::ReactionBase)
-        rj = ReactionType(base=base)
-        rj.base.operatorID = operatorID
-
-        # Add parameters from pars field
-        if hasfield(ReactionType, :pars)
-            add_par(rj, rj.pars)
-        end
-
-        # Add parameters from any additional fields
-        for ef in extra_par_fields
-            add_par(rj, getfield(rj, ef))
-        end
-
-        # optionally set and freeze supplied parameters
-        for (parname, value) in set_pars
-            par = get_parameter(rj, parname)
-            setvalueanddefault!(par, value)
-            setfrozen!(par)
+function find_all_reactions()
+    rtypes = InteractiveUtils.subtypes(AbstractReaction)
+
+    rdict = Dict{String, Type}()
+    duplicate_keys = []
+    for ReactionType in rtypes
+        rname = String(last(split(string(ReactionType), ".")))
+        if haskey(rdict, rname)
+            push!(duplicate_keys, (rname, ReactionType))
         end
+        rdict[rname] = ReactionType
+    end
 
-        return rj
+    for (rname, ReactionType) in duplicate_keys
+        @warn "Duplicate reaction name $rname for Type $ReactionType (removing from Dict)"
+        delete!(rdict, rname)
     end
 
-    return create_init_reaction
+    return rdict
 end
 
 """
-    add_reaction_factory(ReactionType::Type{<:AbstractReaction}; [, operatorID=[1]] [, set_pars::Tuple])
-    add_reaction_factory(classname, ReactionType::Type{<:AbstractReaction}; [, operatorID=[1]] [, set_pars::Tuple])
-    add_reaction_factory(classname, factory_fn)
+    create_reaction(ReactionType::Type{<:AbstractReaction}, base::ReactionBase) -> reaction::AbstractReaction
 
-Register a ReactionType to be created either by using the default [`reaction_factory`](@ref) and typename as classname,
-or by the explicitly supplied `factory_fn` and `classname`.
+Create a `ReactionType` and set `base` field.
 
-Optionally, after creation, set and freeze Parameters in `set_pars` (a Tuple of Pairs of name=>value eg ("total"=>false,) )
+Default implementation may be overriden to eg set additional fields
 """
-function add_reaction_factory(ReactionType::Type{<:AbstractReaction}; kwargs...)
-    add_reaction_factory(string(nameof(ReactionType)), ReactionType; kwargs...)
-    return nothing
-end
-
-function add_reaction_factory(classname::AbstractString, ReactionType::Type{<:AbstractReaction}; kwargs...)
-    add_reaction_factory(classname, reaction_factory(ReactionType; kwargs...))
-    return nothing
+function create_reaction(ReactionType::Type{<:AbstractReaction}, base::ReactionBase)
+    return ReactionType(base=base)
 end
 
-function add_reaction_factory(classname::AbstractString, factory_fn::Function)
-    reaction_factories[classname] = factory_fn
-    return nothing
-end
-
-"""
-    _create_reaction(classname::String, name::String) -> AbstractReaction
-
-Create a new instance of a reaction
-"""
-function _create_reaction(classname::String, name::String, external_parameters::Dict{String, Any})
-
-    # Create a new ReactionXXX instance if a Reaction factory has been registered for this classname
-    if haskey(reaction_factories, classname)
+function create_reaction(rdict::Dict{String, Type}, classname::String, name::String, external_parameters::Dict{String, Any})
+
+    if haskey(rdict, classname)
         base=ReactionBase(name=name, classname=classname, external_parameters=external_parameters)
-        # use factory to create ReactionXXX instance
-        rj = reaction_factories[classname](base)
-        if ! (rj isa AbstractReaction)
-            error("_create_reaction classname='$classname', name='$name' returned ", rj)
+        rj = create_reaction(rdict[classname], base)
+        # Add parameters from pars field
+        if hasproperty(rj, :pars)
+            add_par(rj, rj.pars)
         end
         return rj
     else
@@ -104,6 +60,11 @@ function _create_reaction(classname::String, name::String, external_parameters::
     end
 end
 
+
+function add_reaction_factory(ReactionType::Type{<:AbstractReaction})
+    @warn "call to deprecated add_reaction_factory($ReactionType)"
+end
+
 """
     show_all_reactions(classfilter="", typenamefilter="")
 
@@ -116,16 +77,13 @@ Examples:
 """
 function show_all_reactions(classnamefilter="", typenamefilter="")
 
-    for (classname, ctorfn) in sort(reaction_factories)
+    for (classname, RType) in find_all_reactions()
         if occursin(classnamefilter, classname)
             println(classname)
 
-            base=ReactionBase(name="test", classname=classname, external_parameters=Dict{String, Any}())
-            r = ctorfn(base)       
-            rt = typeof(r)
-            rtstring = Printf.@sprintf("%s", rt)
+            rtstring = Printf.@sprintf("%s", RType)
             if  occursin(typenamefilter, rtstring )      
-                println("    ", rt)
+                println("    ", RType)
 
                 # doc = Base.Docs.doc(rt)  # a Markdown object?
                 # display(doc)

src/reactioncatalog/FluxPerturb.jl

@@ -174,14 +174,4 @@ function do_restore(m::PB.ReactionMethod, (vars, ), cellrange::PB.AbstractCellRa
     return nothing
 end
 
-
-
-"Install create_reactionXXX factories when module imported"
-function __init__()    
-    PB.add_reaction_factory(ReactionFluxPerturb)
-    PB.add_reaction_factory(ReactionRestore)
-
-    return nothing
-end
-
 end

src/reactioncatalog/Fluxes.jl

@@ -469,14 +469,4 @@ function do_transfer(
     return nothing
 end
 
-
-
-"Install create_reactionXXX factories when module imported"
-function __init__()
-    PB.add_reaction_factory(ReactionFluxTarget)
-    PB.add_reaction_factory(ReactionFluxTransfer)
-    return nothing
-end
-
-
 end

src/reactioncatalog/Forcings.jl

@@ -69,12 +69,4 @@ end
 
 
 
-
-"Install create_reactionXXX factories when module imported"
-function __init__()
-    PB.add_reaction_factory(ReactionForceInterp)
-    return nothing
-end
-
-
 end # module

src/reactioncatalog/GridForcings.jl

@@ -445,14 +445,4 @@ function do_force_insolation(m::PB.ReactionMethod, (vars, ), cellrange::PB.Abstr
 end
 
 
-
-
-"Install create_reactionXXX factories when module imported"
-function __init__()
-    PB.add_reaction_factory(ReactionForceGrid)
-    PB.add_reaction_factory(ReactionForceInsolation)
-    return nothing
-end
-
-
 end # module

src/reactioncatalog/GridReactions.jl

@@ -182,12 +182,4 @@ function calc_spherical_area(r, (lond_l, lond_u), (latd_l, latd_u))
 end
 
 
-"Install create_reactionXXX factories when module imported"
-function __init__()
-    PB.add_reaction_factory(ReactionGrid2DNetCDF)
-    PB.add_reaction_factory(ReactionUnstructuredVectorGrid)
-    PB.add_reaction_factory(ReactionCartesianGrid)
-    return nothing
-end
-
 end # module

src/reactioncatalog/Reservoirs.jl

@@ -184,6 +184,14 @@ Base.@kwdef mutable struct ReactionReservoir{P} <: PB.AbstractReaction
     )
 end
 
+abstract type ReactionReservoirTotal <: PB.AbstractReaction end
+function PB.create_reaction(::Type{ReactionReservoirTotal}, base::PB.ReactionBase)
+    rj = ReactionReservoir(base=base)
+    PB.setvalueanddefault!(rj.pars.total, true)
+    PB.setfrozen!(rj.pars.total)
+    return rj
+end
+
 
 function PB.register_methods!(rj::ReactionReservoir)
 
@@ -585,7 +593,7 @@ Base.@kwdef mutable struct ReactionConst{P} <: PB.AbstractReaction
             description="vector of names for constant Variables. Isotopes use <name>::CIsotope syntax"),
     )
 
-    scalar::Bool
+    scalar::Bool = false
 end
 
 function PB.register_methods!(rj::ReactionConst)
@@ -608,35 +616,13 @@ function PB.register_methods!(rj::ReactionConst)
     return nothing
 end
 
-
-function create_ReactionConst(base)
-    rj = ReactionConst(base=base, scalar=false)
-    PB.add_par(rj, rj.pars)
-    return rj
-end
-
-function create_ReactionScalarConst(base)
+abstract type ReactionScalarConst <: PB.AbstractReaction end
+function PB.create_reaction(::Type{ReactionScalarConst}, base::PB.ReactionBase)
     rj = ReactionConst(base=base, scalar=true)
-    PB.add_par(rj, rj.pars)
     return rj
 end
 
 
-"Install create_reactionXXX factories when module imported"
-function __init__()
-    PB.add_reaction_factory(ReactionReservoirScalar)
-    PB.add_reaction_factory(ReactionReservoir, set_pars=("total"=>false,))
-    PB.add_reaction_factory("ReactionReservoirTotal", ReactionReservoir, set_pars=("total"=>true,))
-    PB.add_reaction_factory(ReactionReservoirConst)
-    PB.add_reaction_factory(ReactionReservoirForced)
-    PB.add_reaction_factory(ReactionReservoirWellMixed)
-    PB.add_reaction_factory("ReactionConst", create_ReactionConst)
-    PB.add_reaction_factory("ReactionScalarConst", create_ReactionScalarConst)
-
-    return nothing
-end
-
-
 """
     ReservoirLinksVector(reservoirlist) -> (res::Vector, sms::Vector, diag::Vector)