Add plot for single scenario stratified by size class and functional group #955
Conversation
ConnectedSystems
left a comment
ConnectedSystems
left a comment
There was a problem hiding this comment.
A few minor comments, thanks.
| # as a Label to the whole figure, but left it like this for simplicity | ||
| title = pop!(axis_opts, :title, "") | ||
|
|
||
| for fg in 1:n_groups |
There was a problem hiding this comment.
axes() is recommended over 1:n but I understand this is more readable (with YAXArrays you could iterate over the dimension name, but this isn't the case here.
You can also squash nested loops like so:
for fg in 1:n_groups, sc in 1:n_sizesThere was a problem hiding this comment.
I made this function accept a Array, instead of a YAXArray, because of a few operations that are different with both (like taking the mean`medianpassing the axis name or callinglines!`, that expects an Array). I think it would be nice to accept both (either having two methods one for each of making this one more flexible somehow) but since this is meant for debugging purposes (for now) and the time limit I decided to go only with Arrays for now.
| axis_opts::OPT_TYPE=DEFAULT_OPT_TYPE() | ||
| ) | ||
| n_timesteps, n_sizes, n_groups, n_locations = size(data) | ||
| f = Figure(; size=(1000, 1200), fig_opts...) |
There was a problem hiding this comment.
Arguably, size be part of fig_opts, suggest setting a default value in this method similar to how you're handling ylabel and title below.
| end | ||
| Label(f[0, :], title; fontsize=24) |
There was a problem hiding this comment.
Spacing for clarity.
| end | |
| Label(f[0, :], title; fontsize=24) | |
| end | |
| Label(f[0, :], title; fontsize=24) |
| ydata = mean_data[:, sc, fg] | ||
| lines!(ax, xdata, ydata) |
There was a problem hiding this comment.
I know you're making it more readable but this is more direct and fairly meaningful
| ydata = mean_data[:, sc, fg] | |
| lines!(ax, xdata, ydata) | |
| lines!(ax, xdata, mean_data[:, sc, fg]) |
| function scenarios() end | ||
| function scenarios!() end | ||
|
|
||
| # Plot a single scenario |
There was a problem hiding this comment.
Single scenario of what?
| axis_opts=Dict{Symbol,Any}(:limits=(nothing, (0.0, 1.0))) | ||
| ) | ||
| # Plot scenario cover |
There was a problem hiding this comment.
This seems out of place? Or the example is unfinished?
| # Plot scenario cover | ||
| ``` | ||
| """ | ||
| function ADRIA.viz.scenario_stratified( |
There was a problem hiding this comment.
I don't think scenario_stratified is a good name - there are many ways a scenario can be stratified - but I also understand it's hard to come up with a name that makes sense and isn't overly verbose.
Best alternative I can think of is scenario_by_taxa which at least indicates what will be shown.
There was a problem hiding this comment.
I also don't like scenario_stratified very much. I don't like scenario_by_taxa because it plots by functional group and by size class. And, in the future, this could have a flag so the user decides if they want it plotted by functional group, by size class or both. Alternatively
- We could simple call this
scenario_by_size_and_group(I know, long name...) and in the future have ascenario_by_groupand ascenario_by_size. - We could call it
scenarioand have a flaggroup_bythat can besize_and_group,size,groupornothing.
| # Arguments | ||
| - `data` : Either a 4D array comprising (timesteps, size_classes, functional_groups and | ||
| locations) or a 3D array comprising (timesteps, size_groups, locations). | ||
| - `axis_opts` : Opts to be passed to Makie.Axis. |
There was a problem hiding this comment.
| - `axis_opts` : Opts to be passed to Makie.Axis. | |
| - `axis_opts` : Options to be passed to `Makie.Axis()`. |
Arguably dimension names (timesteps, size_classes, etc.) should also be backticked, but it's a small comment.
| - `data` : Either a 4D array comprising (timesteps, size_classes, functional_groups and | ||
| locations) or a 3D array comprising (timesteps, size_groups, locations). |
There was a problem hiding this comment.
It states 3D or 4D array but the function signature specifies it must be 4D?
This is useful when running a single scenario directly for debugging purposes. A version of that aggregating across all scenarios can be developed in the future as well.
An example of how it looks like for different metrics: