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.

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

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?

src/decision/Criteria/DecisionPreferences.jl

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. :)

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.07317% with 34 lines in your changes missing coverage. Please review.

Project coverage is 51.83%. Comparing base (40d795e) to head (195b56b).

Files with missing lines	Patch %	Lines
ext/AvizExt/viz/spatial.jl	8.69%	21 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   52.72%   51.83%   -0.90%     
==========================================
  Files          72       72              
  Lines        4850     4887      +37     
==========================================
- Hits         2557     2533      -24     
- Misses       2293     2354      +61

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

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

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

Rosejoycrocker · 2024-12-10T03:07:08Z

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

Could you give this a look over too? Thanks @Zapiano :)

Rosejoycrocker · 2025-01-16T22:05:27Z

Could you give this another look @Zapiano? Thanks!

Zapiano · 2025-01-16T22:07:41Z

Could you give this another look @Zapiano? Thanks!

Hey @Rosejoycrocker , it looks like some checks haven't passed because some lines were added but not tested.

Rosejoycrocker · 2025-01-17T00:28:48Z

Could you give this another look @Zapiano? Thanks!

Hey @Rosejoycrocker , it looks like some checks haven't passed because some lines were added but not tested.

Seems like I added tests to a file which is no longer included in runtests.jl (site_selection.jl). I just added them to mcda.jl instead and they're passing locally.

Zapiano · 2025-01-17T00:40:16Z

Could you give this another look @Zapiano? Thanks!

Hey @Rosejoycrocker , it looks like some checks haven't passed because some lines were added but not tested.

Seems like I added tests to a file which is no longer included in runtests.jl (site_selection.jl). I just added them to mcda.jl instead and they're passing locally.

It looks like some tests are not passing :/

…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

Remove incorrect additions from rebase

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

Fix function signatures

Update function signatures

Formatting Put func docstrings on single lines Fixes from rebase

Should be moved to `site_selection.jl` later when all tests are passing Add additional testing to decision matrix visualisation tests Formatting Fix tests

Rosejoycrocker · 2025-01-19T23:40:54Z

Could you give this another look @Zapiano? Thanks!

Hey @Rosejoycrocker , it looks like some checks haven't passed because some lines were added but not tested.

Seems like I added tests to a file which is no longer included in runtests.jl (site_selection.jl). I just added them to mcda.jl instead and they're passing locally.

It looks like some tests are not passing :/

@Zapiano , tests are passing locally for me now but don't seem to be running on here anymore.

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 force-pushed the tmp-add-mcda-visualisations branch from a8c0896 to d72ce02 Compare October 28, 2024 22:48

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 force-pushed the tmp-add-mcda-visualisations branch from 8e5dcb3 to 409bfba Compare November 4, 2024 22:48

Zapiano assigned Rosejoycrocker Nov 12, 2024

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

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from 1a677cc to 195b56b Compare January 16, 2025 21:42

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from 2cc0f2f to a1bb043 Compare January 17, 2025 05:18

Rosejoycrocker added 7 commits January 20, 2025 07:30

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

889e2de

…stance of `map`

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

9ffcef9

…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

cba758b

Updates and fix input types to spatial_map

eb2a714

Split rank_by_index into another function `get_criteria_aggregate'

28adde3

Allows scores to be calculated separately for analysis

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

d0db690

Update example to reflect latest workflow for plotting decision matrices

88d2d9b

Rosejoycrocker added 7 commits January 20, 2025 07:31

Formatting

4bfb398

Remove incorrect additions from rebase

Split into 2 functions

47930bc

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

e4e9cd2

Fix function signatures

Update example and add new plotting functions to viz

ce46260

Fix formatting

fe015ce

Update function signatures

Add conversion to human readable text when plotting

af4fa3b

Add testing criteria visualisations to site selection tests

3b6aeda

Formatting Put func docstrings on single lines Fixes from rebase

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from a1bb043 to 6fa5853 Compare January 19, 2025 21:35

Add decision matrix visualisation test to /test/mcda.jl

bb6b35e

Should be moved to `site_selection.jl` later when all tests are passing Add additional testing to decision matrix visualisation tests Formatting Fix tests

Rosejoycrocker force-pushed the tmp-add-mcda-visualisations branch from 6fa5853 to bb6b35e Compare January 19, 2025 23:35

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

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

Rosejoycrocker commented Dec 10, 2024

Rosejoycrocker commented Jan 16, 2025

Zapiano commented Jan 16, 2025

Rosejoycrocker commented Jan 17, 2025

Zapiano commented Jan 17, 2025

Rosejoycrocker commented Jan 19, 2025 •

edited

Loading

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

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

Rosejoycrocker commented Dec 10, 2024

Rosejoycrocker commented Jan 16, 2025

Zapiano commented Jan 16, 2025

Rosejoycrocker commented Jan 17, 2025

Zapiano commented Jan 17, 2025

Rosejoycrocker commented Jan 19, 2025 • edited Loading

ConnectedSystems Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024 •

edited

Loading

Rosejoycrocker commented Jan 19, 2025 •

edited

Loading