Add option to use JMcDM functions in ADRIA site selection #348

Rosejoycrocker · 2023-05-17T06:31:56Z

Adds option to use either native ADRIA mcda aggregation functions or one of 18 available methods from the JMcDM package. Also adds function retrieve_ranks to allow transitioning between JMcDM methods and ADRIA methods while running ADRIA.

Closes #242

…ference to site ids in decision matrix

…hod vector instead

…ly in final sorting step

…McDM funcs

…n methods vector

…cDM and adria mcda method types

Rosejoycrocker · 2023-05-17T06:54:06Z

Note: I have also reverted the wave_stress criteria normalisation back to min-max as the max normalisation was giving such a small range of values, everything was getting filtered out during testing. This is just a placeholder for whatever we decide to do with this.

ConnectedSystems

Thanks Rose, much tidier well done.

This is a partial review, will continue on tomorrow.

ConnectedSystems · 2023-05-17T09:13:11Z

src/sites/dMCDA.jl

+- `weights` : importance weights for each criteria. 
+- `mcda_func` : function/[function bool] array to use for mcda, specified as an element from method.
+- `site_ids` : array of site ids still remaining after filtering.
+- `scores` : set of scores derived from applying an mcda ranking method.


Orphaned arguments in argument list?

Please read the argument list and then compare with the function signatures given at the top, and the function signature that is implemented.

List of functions in docstring still doesn't match implementations?

@Rosejoycrocker I guess you did update them but they're still incorrect.

Corrected, and changed rev_val to maximize as the latter is more intuitive.

src/sites/dMCDA.jl

ConnectedSystems · 2023-05-17T09:29:36Z

src/sites/dMCDA.jl

 end
+function retrieve_ranks(S::Matrix, scores::Vector, rev_val::Bool, site_ids::Vector)
+
+    s_order = Union{Float64,Int64}[Int.(site_ids) scores 1:size(S, 1)]


Use concrete types where possible please: Int ->Int64

And you can consolidate this line with Line 243 to avoid reallocations.

ConnectedSystems · 2023-05-17T09:31:26Z

src/sites/dMCDA.jl

+using JMcDM
+using ADRIA: order_ranking, adria_vikor, adria_topsis
+
+global method = [


Globals are bad! Mark this a const for the module please.

Also, isn't there a way of getting a list of available methods from JMcDM?

The list of available methods wasn't working for me when I tried previously. I just tried again and it's working now, so I'll add it.

Actually, sorry I remember now why I used the global (which I can replace with a constant), it's because I needed to add something which indicates whether the method gives best sites as the highest score or lowest score (I add a Boolean next to the method) and I wasn't sure how to do this without manually looking at the docs for the method and working it out. I'll have a look again and see if there is something which indicates but otherwise not sure how to replace.

So looks like the results struture also has a "best index" field, so I can add somethng which checks whether the best index is equal to the max or min score and order the sites accordingly :)

So I think the reason the list of JMcDM functions wasn't working for me before is it uses the subtypes() function (in subtypes(MCDMMethod)) which is automatically loaded in interactive mode but doesn't seem to be loaded when running ADRIA. Due to this it throws an error saying it can't find the subtypes() function. Is it ok to include the InteractiveUtils module in dMCDA or is this not a good idea? If so, is there a way to just load one function from the module?

Note, I've also tried methods(mcda_func)[1] to extract the method but because it's a general method and the mcda function requires a JMcDM.MCDMMethod type it errors.

https://github.com/open-AIMS/ADRIA.jl/blob/ea5762f5b1f050a1f031ef9a538acb3741237329/src/sites/dMCDA.jl#L222C10-L229

That's what you want.

Note that the Promethee method errors because the constructor for it expects additional arguments.
There may be others like it.

Not sure what you want to do with these methods.

Also note the docstring for retrieve_ranks is now out of date.

ERROR: MethodError: no method matching JMcDM.PROMETHEE.PrometheeMethod() Closest candidates are: JMcDM.PROMETHEE.PrometheeMethod(::Vector{<:Function}, ::Vector{Float64}, ::Vector{Float64}) @ JMcDM C:\Users\takuy\.julia\packages\JMcDM\Oo88a\src\promethee.jl:22 | ETA: 0:00:04 JMcDM.PROMETHEE.PrometheeMethod(::Vector{Function}, ::Vector{Union{Nothing, Float64}}, ::Vector{Union{Nothing, Float64}}) @ JMcDM C:\Users\takuy\.julia\packages\JMcDM\Oo88a\src\promethee.jl:13 JMcDM.PROMETHEE.PrometheeMethod(::Any, ::Any, ::Any) @ JMcDM C:\Users\takuy\.julia\packages\JMcDM\Oo88a\src\promethee.jl:13

Ah man, brackets is all it needed :( ...
Promethee should be in the list of excluded methods because of this- it's more complicated because you get 2 scores are output and based on these you decide whether to have a single or multiple "best choice". I'll add it to the list. The others which I've added should be the only methods which are exceptions, I looked through all th docs to catch any which have non-standard outputs.

I'll update the docstring.

I did say to call the constructor...

#348 (comment)

I didn't realise that's what you meant by your example, I thought you were just referring to the datatype of the inputs because you referred to multiple dispatch

The constructed method should be passed into `mcdm()`, not the datatype itself.

Rosejoycrocker · 2023-05-23T02:54:38Z

Hey @ConnectedSystems I think I've resolved your requested changes now. Let me know if there is anything else. I haven't changed S to be a NamedDims+AxisKeys array because this was a planned change for when replacing mcda_vars which I will do in another PR

ConnectedSystems · 2023-05-23T03:18:04Z

Thanks, will have a look this afternoon

ConnectedSystems · 2023-05-23T08:22:49Z

src/sites/dMCDA.jl

+    order_ranking,
+    adria_vikor,
+    adria_topsis,
+    setdiff(jmcdm_methods, jmcdm_ignore)...


FYI @Rosejoycrocker thought of a cleaner way to ignore unwanted methods.

ConnectedSystems · 2023-05-23T08:24:44Z

src/sites/dMCDA.jl

+- `weights` : importance weights for each criteria. 
+- `mcda_func` : function/[function bool] array to use for mcda, specified as an element from method.
+- `site_ids` : array of site ids still remaining after filtering.
+- `scores` : set of scores derived from applying an mcda ranking method.


List of functions in docstring still doesn't match implementations?

ConnectedSystems · 2023-05-23T08:25:47Z

src/sites/mcda_methods.jl

+    Q .= 1.0 .- Q  # Invert rankings so higher values = higher rank
+
+    return Q
+end


You need to add an empty line at the very end to stop the linter from complaining

(I'll explain why leaving an empty line at the end is a common convention in programming later).

Sorry, missed this when copying and pasting

List of functions in docstring still doesn't match implementations?

I thought I did this in commit e5ea28 , called "Change order of doc input values to match order"... maybe it got overwritten when merged with Main?

ConnectedSystems · 2023-05-23T08:28:57Z

src/sites/dMCDA.jl

@@ -3,6 +3,10 @@
 using StatsBase
 using Distances
 using Combinatorics
+using JMcDM
+using InteractiveUtils: subtypes
+using ADRIA: order_ranking, adria_vikor, adria_topsis


Quick question - was the adria_ prefix necessary?

Looks a bit tacky in the sense that none of the other methods have the prefix.
At first glance it doesn't seem like vikor or topsis would have resulted in a clash with JMcDM functions, so wondering if we can save the extra keypresses.

I originally didn't have the prefix but JMcDM seems to have wrapping functions for the vikor and topsis methods so instead of mcda(A,weights, method()) you can do vikor(A,weights) etc... so I had to have something different

Okay. I still think it looks a bit clunky but leave it as is. It'll be removed in the future if you contribute changes to JMcDM anyway.

ConnectedSystems

Thanks for this PR Rose!

Rosejoycrocker added 18 commits May 17, 2023 11:13

Add JMcDM to ADRIA dependencies

66954ef

Change ADRIA mcda method names to avoid clash with JMcDM function names

b5fe689

Changes to retrieve_ranks to allow compatibility with JMcDM functions

74f0db6

Add method struct which allows selection of JMcDM funcs for mcda_func

8cd9e8a

Type changes to retrieve_ranks inputs

d47a735

Remove moosra method, needs more research to be implemented properly

ab3e8a5

Add docs for retrieve_ranks function

9067d09

Add ADRIA mcda methods to methods vector

614c611

Remove ranking from inside ADRIA mcda ranking functions and remove re…

8a97cd0

…ference to site ids in decision matrix

Remove elseif statements for deciding mcda method and select from met…

c54bf1d

…hod vector instead

Changes to unify mcda method types. Remove site_ids from S and use on…

a5d26d1

…ly in final sorting step

Changes to rank_sites to allow simple interchange between ADRIA and J…

b4a1f11

…McDM funcs

Change location of rank_sites!

cafc1ad

Put mcda methods for ADRIA into separate file so they can be called i…

d2bc7d7

…n methods vector

Change how mcda_func is used in rank_sites, remove mcda method funcs

7f5885b

Fix rank_sites! documentation

4eb7cc2

Fix retrieve_ranks docs and changes to input types to unify across JM…

5e946b4

…cDM and adria mcda method types

Change output type of mcda methods as no longer uses site ids in S

d0d9e19

Rosejoycrocker requested a review from ConnectedSystems May 17, 2023 06:31

Rosejoycrocker added the enhancement New feature or request label May 17, 2023

ConnectedSystems requested changes May 17, 2023

View reviewed changes

Rosejoycrocker and others added 8 commits May 18, 2023 15:31

Simplifications to retrieve_ranks + change order of inputs

849224d

Add InteractiveUtils to dependencies

2e520fc

Add usage of JMcDM function list

99d88a2

Remove use of vector for setting JMcDM sorting methods

b119fa9

Spacing and standardize comment format

6f48895

Expected argument is a DataType

3e7ee06

The constructed method should be passed into `mcdm()`, not the datatype itself.

Clean up wall of text

ea5762f

Add PrometheeMethod to list of excluded methods

b5093fd

Rosejoycrocker added 4 commits May 23, 2023 11:49

Update docs for retrieve_ranks

ca434a3

Add indexation to compare scalar max not vector (will always give false)

dc47465

Change order of docs input values to match change in order

e5ea28c

Change Int to Int64 in rank_sites!

462e8ad

Simplify removal of JMcDM methods to ignore

8e3059d

ConnectedSystems reviewed May 23, 2023

View reviewed changes

ConnectedSystems and others added 4 commits May 23, 2023 18:44

Merge branch 'main' into add-JMcDM-to-mcda

06094f4

Add line at end of mcda_methods()

72e7846

whitespace

0531dfa

Update docstring and use more intuitive argument name

6b7a72f

ConnectedSystems approved these changes May 24, 2023

View reviewed changes

ConnectedSystems merged commit 23af223 into main May 24, 2023

ConnectedSystems deleted the add-JMcDM-to-mcda branch May 24, 2023 02:20

ConnectedSystems mentioned this pull request May 24, 2023

MCDA method errors #357

Closed

Zapiano mentioned this pull request Jun 26, 2023

Register v0.6.0 #379

Closed

JuliaRegistrator mentioned this pull request Jun 26, 2023

New version: ADRIA v0.6.0 JuliaRegistries/General#86244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to use JMcDM functions in ADRIA site selection #348

Add option to use JMcDM functions in ADRIA site selection #348

Rosejoycrocker commented May 17, 2023

Rosejoycrocker commented May 17, 2023

ConnectedSystems left a comment •

edited

Loading

ConnectedSystems May 17, 2023

Rosejoycrocker May 17, 2023

ConnectedSystems May 18, 2023 •

edited

Loading

ConnectedSystems May 23, 2023

ConnectedSystems May 24, 2023

ConnectedSystems May 17, 2023

ConnectedSystems May 17, 2023

Rosejoycrocker May 17, 2023

Rosejoycrocker May 17, 2023

Rosejoycrocker May 17, 2023

Rosejoycrocker May 17, 2023

Rosejoycrocker May 23, 2023

ConnectedSystems May 23, 2023 •

edited

Loading

Rosejoycrocker May 23, 2023

ConnectedSystems May 23, 2023

Rosejoycrocker May 23, 2023 •

edited

Loading

Rosejoycrocker commented May 23, 2023

ConnectedSystems commented May 23, 2023

ConnectedSystems May 23, 2023

ConnectedSystems May 23, 2023

ConnectedSystems May 23, 2023

Rosejoycrocker May 23, 2023

Rosejoycrocker May 23, 2023

ConnectedSystems May 23, 2023

Rosejoycrocker May 23, 2023

ConnectedSystems May 23, 2023

ConnectedSystems left a comment

Add option to use JMcDM functions in ADRIA site selection #348

Add option to use JMcDM functions in ADRIA site selection #348

Conversation

Rosejoycrocker commented May 17, 2023

Rosejoycrocker commented May 17, 2023

ConnectedSystems left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnectedSystems May 18, 2023 • edited Loading

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

Choose a reason for hiding this comment

ConnectedSystems May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rosejoycrocker May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Rosejoycrocker commented May 23, 2023

ConnectedSystems commented May 23, 2023

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 left a comment

Choose a reason for hiding this comment

ConnectedSystems left a comment •

edited

Loading

ConnectedSystems May 18, 2023 •

edited

Loading

ConnectedSystems May 23, 2023 •

edited

Loading

Rosejoycrocker May 23, 2023 •

edited

Loading