-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add spatial visualisations for decision matrices #893
Open
Rosejoycrocker
wants to merge
14
commits into
main
Choose a base branch
from
tmp-add-mcda-visualisations
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+293
−28
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
6ef0899
Move `viz.decision_matrices` into `spatial.jl` and make it another in…
Rosejoycrocker 2d9f37c
Fix how scores are plotted (should be inside loop), add use of human …
Rosejoycrocker 25d2890
Update criteria map plotting method in docs + update example fig
Rosejoycrocker 833a3c5
Updates and fix input types to spatial_map
Rosejoycrocker 9d27f72
Split `rank_by_index` into another function `get_criteria_aggregate'
Rosejoycrocker 05def0f
Updates to viz.map for plotting decision matrices for latest viz version
Rosejoycrocker 2feceff
Update example to reflect latest workflow for plotting decision matrices
Rosejoycrocker 7e26a3e
Formatting
Rosejoycrocker 1c52cd3
Split into 2 functions
Rosejoycrocker 2d18fa4
Change name of `get_criteria_aggregate` to `criteria_aggregate_scores`
Rosejoycrocker d2f4772
Update example and add new plotting functions to viz
Rosejoycrocker 409bfba
Fix formatting
Rosejoycrocker 01a997c
Add conversion to human readable text when plotting
Rosejoycrocker 1a677cc
Add testing criteria visualisations to site selection tests
Rosejoycrocker File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
map
is a very generic name, I don't think we should make a method calledmap
plot a specific kind of map only (only maps of decision matrix scores). What I mean is: it is interesting to have amap
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 amap
function that receives this matrix and thetitles_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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
orvizualize_selection_criteria_as_map
ordisplay_map
orplot_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:
map()
produces a single map.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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 calledprime_number_a
andprime_number_b
. If it sums more than just prime numbers, they should be callednumber_a
andnumber_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 thismap
function with multiple maps is defined we can just use this one and that's fine. My comment was more about making thismap
general than about the need of a specific function with a specific name for this kind of plot.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.