open-AIMS / ADRIA.jl

ADRIA: Adaptive Dynamic Reef Intervention Algorithms. A multi-criteria decision support platform for informing reef restoration and adaptation interventions.
MIT License
14 stars 5 forks source link

Fix visualization type error #777

Closed DanTanAtAims closed 3 months ago

DanTanAtAims commented 3 months ago
example_err

The update to use Dict{Symbol,<:Any} in ADRIA.viz.map for optional arguments caused type mismatches in visualisations that were using mapping. Specifically, spatial visualization of clustering results.

This pull requests fixes this mismatch and allows tests to be run without erroring again.

ConnectedSystems commented 3 months ago

As you mentioned in a different conversation, we might as well migrate all use of Dict() to an internally defined const of Dict{Symbol,<:Any}.

We could also define an empty dict for all methods to use as a default value.

Something like:

const OPT_TYPE = Dict{Symbol,<:Any}
const DEFAULT_OPT_VAL = Dict{Symbol,Any}()

function some_viz_function(x, y; opt::OPT_TYPE=DEFAULT_OPT_VAL)
end

This should work...

Zapiano commented 3 months ago

Is there a reason why we don't use NamedTuples instead of Dicts for this?

ConnectedSystems commented 3 months ago

Is there a reason why we don't use NamedTuples instead of Dicts for this?

Tuples are immutable, but there are places where options need to be added/updated.

julia> x = (pedro=100, award=true)
(pedro = 100, award = true)

julia> x.another_option = 200
ERROR: type NamedTuple has no field another_option
Stacktrace:
 [1] setproperty!(x::@NamedTuple{pedro::Int64, award::Bool}, f::Symbol, v::Int64)
   @ Base .\Base.jl:39
 [2] top-level scope
   @ REPL[10]:1

Hypothetically you could use NamedTuples by recreating a NamedTuple everytime a method needs to add/update an option, but it will get messy...

Using Dicts was the most straightforward approach I could think of at the time. Open to other options.

DanTanAtAims commented 3 months ago

Something I found unexpected with DEFAULT_OPT_VAL is although its declared constant, functions that edit the opt dictionary creates side effects for others. For example

ADRIA.viz.clustered_scenarios(args...; opts=DEFAULT_OPT_VAL)

contains the following line

opts[:histogram] = false

Then if you call any other function with default axis options,

ADRIA.viz.scenarios(rs, s_tac) # axis_opts=DEFAULT_OPT_VAL

it will error as :histogram is not a valid kwarg for the Axis constructor. Somehow we obtain a non-constant reference to a constant global which seems like a bug in Julia.

An alternative is to instead define the default type

const OPT_TYPE = Dict{Symbol,<:Any}
const DEFAULT_OPT_TYPE= Dict{Symbol,Any}

and use

function viz_func(;
    opts::OPT_TYPE=DEFAULT_OPT_TYPE()
)
ConnectedSystems commented 3 months ago

Ah, the constant value itself is mutable. Same with Vectors, you can declare a Vector and add to it, but you can never replace it.

I think your alternative approach is what I actually meant to do...

DanTanAtAims commented 3 months ago

Ready for review.