Add spatial visualisations for decision matrices #893

Rosejoycrocker · 2024-10-25T06:20:07Z

Adds method to viz.map for plotting decision matrices and aggregate scores on a set of side-by-side maps.

Rosejoycrocker · 2024-10-25T06:21:41Z

Here is the plot from running the example code in analysis.md:

(any advice on what the grey boxes are appreciated, I'll try to remove)

Zapiano · 2024-10-28T22:31:25Z

@Rosejoycrocker this PR has a conflict and the tests are not passing. About the tests, this could be due to a problem that was fixed yesterday, not sure.

…stance of `map`

…readable criteria names for labels Plus add criteria names as optional inputs so less than total set of criteria can be plotted

Allows scores to be calculated separately for analysis

Rosejoycrocker · 2024-10-28T23:43:50Z

@Rosejoycrocker this PR has a conflict and the tests are not passing. About the tests, this could be due to a problem that was fixed yesterday, not sure.

I'm looking into it now

Remove incorrect additions from rebase

Rosejoycrocker · 2024-10-29T03:46:52Z

Tests are passing and conflicts reolved now @Zapiano :)

Zapiano · 2024-11-04T03:04:13Z

src/decision/Criteria/DecisionPreferences.jl

@@ -131,6 +131,30 @@ Index of locations ordered by their rank
 function rank_by_index(
    dp::T, dm::YAXArray, method::Union{Function,DataType}
 )::Vector{Int64} where {T<:DecisionPreference}
+    res = get_criteria_aggregate(dp, dm, method)


I imagine res comes from result, right? Everything any function returns is a result (the result of that function), so calling a variable result is a little bit vague (maybe with a few exceptions). Maybe calling it aggregate_score or something else that helps the reader would be better.

Its a result structure that the MCDA package uses, so it contains more than just the aggregate score. Maybe change it to decision_results or something?

Zapiano · 2024-11-04T03:05:18Z

src/decision/Criteria/DecisionPreferences.jl

@@ -131,6 +131,30 @@ Index of locations ordered by their rank
 function rank_by_index(


This function signature doesn't match the docstring (I know this wasn't updated by you, if you prefer just open an issue to fix that on a separate PR).

Zapiano · 2024-11-04T03:08:23Z

src/decision/Criteria/DecisionPreferences.jl

+# Returns
+Returns raw aggreagted score for a set of locations
+"""
+function get_criteria_aggregate(


You say this function returns a raw aggregated score for a set of locations, so maybe it could be called aggregated_scores or criteria_aggregated_scores? I just think this get part of the name may be a little redundant. And, since you are aggregating scores, adding the word scores at the end may clarify what this returns.

The function signature doesn't match the docstring.

Thanks for spotting, I'll fix the signature and change the name to criteria_aggregated_scores

Zapiano · 2024-11-04T03:25:03Z

ext/AvizExt/viz/spatial.jl

@@ -251,6 +256,75 @@ function ADRIA.viz.map!(
        axis_opts
    )
 end
+function ADRIA.viz.map(


Since map is a very generic name, I don't think we should make a method called map plot a specific kind of map only (only maps of decision matrix scores). What I mean is: it is interesting to have a map function that plots multiple maps like you did, but not only for decision matrix. I could plot the average of some different metrics for each location, for example.

So, I propose we have an intermediate function selection_criteria_map, for example, that receives this matrix and this vector, concatenate them to become just a single matrix; builds up a tuple of titles for each plot; and call a map function that receives this matrix and the titles_tuple (and whatever else it needs) and plots this grid of maps. Then, in the future, if we want to plot other multiple map figure, it can be easily reused.

What do you think?

Sounds good :)

I prefer the current approach but I'm not 100% up on the context here, just latching onto the suggestion to create a separate map plotting function. Julia is not strictly an object oriented language and isn't intended to be used like Python or Java.

The advantage of having multiple dispatch (or something like it, as in R) is to prevent everyone from having to remember a lot of function names. Is it map or vizualize_selection_criteria_as_map or display_map or plot_selection_criteria_map or...

As a user I don't want to have to remember endless possible function names for specific use cases. I just want a map.

Instead, this behaviour of:

You have a vector of results -> map() produces a single map.

You have a matrix of results -> map() produces a series of maps.

is sensible to me.

But again, I don't have full context.

https://www.juliaopt.org/meetings/santiago2019/slides/stefan_karpinski.pdf

I understand and I agree with what you said.

My problem with how this function is defined right now is that it doesn't plot selection criteria scores in particular. It plots multiple maps with whatever you pass as an argument. But still the names of variables and the plots default titles are related to this specific kind of plot (multiple maps with selection criteria scores). It's like having a function called sum and the arguments are called prime_number_a and prime_number_b. If it sums more than just prime numbers, they should be called number_a and number_b or whatever.

About the need for a specific function called selection_criteria_map, maybe we don't need one, I agree. depending on how this map function with multiple maps is defined we can just use this one and that's fine. My comment was more about making this map general than about the need of a specific function with a specific name for this kind of plot.

Incidentally @Rosejoycrocker the variable names/titles displayed in the figure should be run through human_readable() (this isn't the exact function name but you should be able to find it easily)

I'm not sure what the outcome of this discssuion is, but I've separated this into a generic map() method which plots multiple maps in a grid given a matrix of inputs, and then a function in location_selection.jl that plots criteria and an aggregate score given a decision matrix input and vector of scores (using the generic map() function). Not sure what you think but I think being able to quickly plot criteria and an output score is useful, hence its inclusion in location_selection.jl.

Zapiano · 2024-11-04T03:26:25Z

ext/AvizExt/viz/spatial.jl

@@ -251,6 +256,75 @@ function ADRIA.viz.map!(
        axis_opts
    )
 end
+function ADRIA.viz.map(
+    rs::Union{Domain,ResultSet},
+    M::YAXArray,


This doesn't match the docstring.

I am against naming variables with a single letter except in very specific cases. You have little to gain and everyone that reads you code (including your future self) have a lot of time to lose with this. :)

One is generic matrix mapping, the other is specifically for mapping criteria and aggregate scores

Fix function signatures

Update function signatures

Rosejoycrocker · 2024-11-04T22:51:37Z

Most recent version of the plotting output (grid has been removed)

codecov · 2024-11-04T23:07:40Z

Codecov Report

Attention: Patch coverage is 17.94872% with 32 lines in your changes missing coverage. Please review.

Project coverage is 51.14%. Comparing base (ad39467) to head (1a677cc).

Files with missing lines	Patch %	Lines
ext/AvizExt/viz/spatial.jl	9.52%	19 Missing ⚠️
ext/AvizExt/viz/location_selection.jl	0.00%	11 Missing ⚠️
src/viz/viz.jl	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #893      +/-   ##
==========================================
- Coverage   51.44%   51.14%   -0.31%     
==========================================
  Files          72       72              
  Lines        4782     4818      +36     
==========================================
+ Hits         2460     2464       +4     
- Misses       2322     2354      +32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Formatting Put func docstrings on single lines

Rosejoycrocker · 2024-11-13T03:45:35Z

I think I've addressed your comments @Zapiano , let me know if you have others

Rosejoycrocker requested a review from Zapiano October 25, 2024 06:20

Rosejoycrocker mentioned this pull request Oct 25, 2024

Add functions to spatially visualise individual and aggregate criteria for location selection #650

Closed

Rosejoycrocker added 7 commits October 29, 2024 08:47

Move viz.decision_matrices into spatial.jl and make it another in…

6ef0899

…stance of `map`

Fix how scores are plotted (should be inside loop), add use of human …

2d9f37c

…readable criteria names for labels Plus add criteria names as optional inputs so less than total set of criteria can be plotted

Update criteria map plotting method in docs + update example fig

25d2890

Updates and fix input types to spatial_map

833a3c5

Split rank_by_index into another function `get_criteria_aggregate'

9d27f72

Allows scores to be calculated separately for analysis

Updates to viz.map for plotting decision matrices for latest viz version

05def0f

Update example to reflect latest workflow for plotting decision matrices

2feceff

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from a8c0896 to d72ce02 Compare October 28, 2024 22:48

Formatting

7e26a3e

Remove incorrect additions from rebase

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from 16daef5 to 7e26a3e Compare October 29, 2024 02:53

Zapiano reviewed Nov 4, 2024

View reviewed changes

Rosejoycrocker added 4 commits November 5, 2024 08:35

Split into 2 functions

1c52cd3

One is generic matrix mapping, the other is specifically for mapping criteria and aggregate scores

Change name of get_criteria_aggregate to criteria_aggregate_scores

2d18fa4

Fix function signatures

Update example and add new plotting functions to viz

d2f4772

Fix formatting

409bfba

Update function signatures

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from 8e5dcb3 to 409bfba Compare November 4, 2024 22:48

Add conversion to human readable text when plotting

01a997c

Zapiano assigned Rosejoycrocker Nov 12, 2024

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from 15e7273 to 841bece Compare November 13, 2024 03:17

Add testing criteria visualisations to site selection tests

1a677cc

Formatting Put func docstrings on single lines

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from 841bece to 1a677cc Compare November 13, 2024 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spatial visualisations for decision matrices #893

Add spatial visualisations for decision matrices #893

Rosejoycrocker commented Oct 25, 2024

Rosejoycrocker commented Oct 25, 2024

Zapiano commented Oct 28, 2024

Rosejoycrocker commented Oct 28, 2024

Rosejoycrocker commented Oct 29, 2024

Zapiano Nov 4, 2024

Rosejoycrocker Nov 4, 2024

Zapiano Nov 4, 2024

Zapiano Nov 4, 2024

Rosejoycrocker Nov 4, 2024

Zapiano Nov 4, 2024

Rosejoycrocker Nov 4, 2024

ConnectedSystems Nov 6, 2024

Zapiano Nov 6, 2024

ConnectedSystems Nov 6, 2024 •

edited

Loading

Rosejoycrocker Nov 6, 2024

Zapiano Nov 4, 2024

Rosejoycrocker commented Nov 4, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

Rosejoycrocker commented Nov 13, 2024

		@@ -131,6 +131,30 @@ Index of locations ordered by their rank
		function rank_by_index(

Add spatial visualisations for decision matrices #893

Are you sure you want to change the base?

Add spatial visualisations for decision matrices #893

Conversation

Rosejoycrocker commented Oct 25, 2024

Rosejoycrocker commented Oct 25, 2024

Zapiano commented Oct 28, 2024

Rosejoycrocker commented Oct 28, 2024

Rosejoycrocker commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnectedSystems Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rosejoycrocker commented Nov 4, 2024

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

Rosejoycrocker commented Nov 13, 2024

ConnectedSystems Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024 •

edited

Loading