Bug fixes, compartmentalisation and documentation for priority predecessor and zones criteria #634

Rosejoycrocker · 2023-12-19T05:14:41Z

Closes #633

Separates the priority locations and priority zones criteria into their own functions for ease of testing. Also add further documentation for these criteria in the function docs.

Adds a testset for these criteria constructor functions. Also adds updates for the mcda decision matrix construction tests which were previously not included in the test suite.

test/mcda.jl

ConnectedSystems · 2024-01-11T02:55:43Z

Could you give an overview of what bugs were fixed?

Rosejoycrocker · 2024-01-11T03:28:04Z

Could you give an overview of what bugs were fixed?

Sure, it's briefly summarized in #633, but I'll describe it more here. In the Matlab version the strong_pred variable is a 2*nsites matrix : https://github.com/open-AIMS/ADRIA_repo/blob/bc29474555015b88fa0a10f7f935408f0b4f0e76/ADRIAfunctions/siteConnectivity.m#L197
where the first column is the site ids. In the Julia version it is a vector :

ADRIA.jl/src/ecosystem/connectivity.jl

Line 190 in 5f7db89

strong_pred = zeros(Int64, size(C1)...)

When strong_pred is used in predec, the structure used to calculate the priority sites criteria, it is still used in the same way as the matlab version-

Matlab: https://github.com/open-AIMS/ADRIA_repo/blob/bc29474555015b88fa0a10f7f935408f0b4f0e76/ADRIAfunctions/ADRIA_DMCDA.m#L66-L69

Julia:

ADRIA.jl/src/decision/dMCDA.jl

Lines 597 to 599 in 5f7db89

    
           predec::Matrix{Float64} = zeros(n_sites, 3) 
        
           predec[:, 1:2] .= strong_pred 
        
           predprior = predec[in.(predec[:, 1], [priority_sites']), 2]

This means L599 above in the Julia version is using predec[1,:] assuming it is a list of site ids, when it is filled with strong_pred on L598, where 2 copies of the vector strong_pred are added to predec.

This difference informed assumptions in how the zone criteria are calculated also, so this PR includes changes to the zone criteria.

ConnectedSystems · 2024-01-11T04:02:52Z

Ah okay, sorry I somehow missed the linked issue. But the additional info is nice.

ConnectedSystems · 2024-01-11T07:04:49Z

Please format all files to conform to the style guide

Rosejoycrocker · 2024-01-11T22:00:06Z

Please format all files to conform to the style guide

I just formatted, so should be good to go

ConnectedSystems · 2024-01-23T23:26:36Z

src/analysis/scenario.jl

    rcps::Vector{Symbol} = Symbol.(:RCP, Int64.(scenarios[:, :RCP]))
    return Dict(rcp => rcps .== rcp for rcp in unique(rcps))
 end

-function scenario_types(scenarios::DataFrame)::Dict{Symbol,BitVector}
+function scenario_types(scenarios::DataFrame)::Dict{Symbol, BitVector}


Can you standardise this formatting rule as well please?

Dict{Symbol, BitVector} should have no space between the types.

ConnectedSystems · 2024-01-23T23:27:00Z

src/decision/dMCDA.jl

@@ -333,7 +336,8 @@ Columns indicate:
 - `wave_stress` : Probability of wave damage.
 - `heat_stress` : Probability of site being affected by heat stress
 - `predec` : List of priority predecessors (sites strongly connected to priority sites).
- `risk_tol` : Tolerance for wave and heat risk (∈ [0,1]). Sites with heat or wave risk> risk_tol are filtered out.
+- `risk_tol` : Tolerance for wave and heat risk (∈ [0,1]). Sites with heat or wave 
+    risk> risk_tol are filtered out.


Typo

Suggested change

risk> risk_tol are filtered out.

risk > risk_tol are filtered out.

ConnectedSystems · 2024-01-23T23:28:03Z

src/ecosystem/connectivity.jl

@@ -187,7 +188,7 @@ function connectivity_strength(TP_base::AbstractMatrix{Float64})::NamedTuple
    C2 = outdegree_centrality(g)

    # For each node, find strongly connected predecessor (by number of connections)
-    strong_pred = zeros(Int64, size(C1)...)
+    strong_pred = zeros(Int64, (size(C1)...))


Did you add the parenthesis in or was this the formatter?

I think this would work without the splat or parenthesis

Suggested change

strong_pred = zeros(Int64, (size(C1)...))

strong_pred = zeros(Int64, size(C1))

ConnectedSystems · 2024-02-09T03:55:56Z

Hi Rose, could I get an update on this?

I still see style issues but perhaps you've addressed these and have not yet pushed the changes?

Rosejoycrocker · 2024-02-09T04:17:48Z

Hi Rose, could I get an update on this?

I still see style issues but perhaps you've addressed these and have not yet pushed the changes?

Hi Takuya, I recently pushed formatting changes but I just copied from the formatting setup guide you and Dan just added because I've noticed some things are a bit different. I'm just about to go and reformat all my PRs

ConnectedSystems · 2024-02-12T22:00:28Z

Hi Rose, could you rebase this onto main please?

Rosejoycrocker · 2024-02-13T02:01:00Z

In priority sites we use strong_pred to favour sites that are the strongest sources of the listed priority sites, and in the priority zones criteria we use strong_pred to favour sites which are the strongest sources to a zone. strong_pred is not used directly as a criteria. Do you mean not appropriate as a measure of larval connectivity?

ConnectedSystems · 2024-02-13T02:20:57Z

In priority sites we use strong_pred to favour sites that are the strongest sources of the listed priority sites, and in the priority zones criteria we use strong_pred to favour sites which are the strongest sources to a zone. strong_pred is not used directly as a criteria. Do you mean not appropriate as a measure of larval connectivity?

No no, I missed that it wasn't using strong_pred directly inside guided_site_selection(), but was going through two additional steps.

Rosejoycrocker · 2024-02-13T02:30:48Z

Yeah it's a bit confusing because strong_pred is what is stored in mcda_vars. This PR separates the processing of strong_pred to create the zone and priority sites criteria into 2 functions (previously it was just done inside guided_site_selection). So priority_location_criteria() adds proportional value to sites that are the strongest sources of sites in the priority list by indexing strong_pred at the priority site locations and then using those resulting indices to add value to the priority criteria. It's similar for the zones criteria (calculated in priority_zones_criteria()) but value is added to the whole zone and to any sites which are the strongest source for sites in the zone.

It also fixes a bug which was occuring because strong_pred used to be a matrix with site ids in the first column and strongest source sites in the second column. The methods were still assuming it had this structure when it no longer does so I've made changes to account for this.

…dataframe

…er exists, to be updated shortly in another PR)

Remove `factors` input from `rsa()` and use scenario dataframe column names instead

…rate functions

…ity locations functions `strong_pred` and `zones` must be the same size

Zones criteria was incorrectly indexing using `strong_pred` - should have been using sites in `strong_pred` which contain priority zone sites, not values of `strong_pred` at sites in priority zones

…es_criteria`

`strong_pred` should be a matrix, with site ids in the first column. Without this, calcs in priority predecessor and zones criteria are incorrect

…g in a zone matters

Simplify and fix how `strong_pred` is used in zoning criteria Add to docs

Remove unnecessary findall Remove unnecessary findall

…additive, not replacement)

…on matrix tests

…rces and zone members

…priority zone have the highest criteria value

…reation + some formatting

…n docs Fix formatting and style

Was checking column 6 which is priority predecessors, instead of 8 which is leftover space

Formatting

…t work

ConnectedSystems · 2024-02-21T07:02:42Z

I'll be merging this into the changes for MCDA Rose

Rosejoycrocker requested a review from ConnectedSystems December 19, 2023 05:14

Rosejoycrocker added the bug Something isn't working label Dec 19, 2023

ConnectedSystems reviewed Dec 22, 2023

View reviewed changes

test/mcda.jl Outdated Show resolved Hide resolved

Rosejoycrocker self-assigned this Jan 4, 2024

Rosejoycrocker force-pushed the separate-zone-predec-crit branch from 6bbeb21 to d9a4cba Compare January 10, 2024 03:22

ConnectedSystems force-pushed the separate-zone-predec-crit branch from 48553f6 to 0470f52 Compare January 11, 2024 02:49

ConnectedSystems force-pushed the separate-zone-predec-crit branch from 0470f52 to 87767c0 Compare January 11, 2024 07:04

Rosejoycrocker force-pushed the separate-zone-predec-crit branch 4 times, most recently from 2246d40 to d1e96de Compare January 11, 2024 21:36

Rosejoycrocker force-pushed the separate-zone-predec-crit branch 2 times, most recently from d18662a to 8a68c7a Compare January 23, 2024 22:48

ConnectedSystems reviewed Jan 23, 2024

View reviewed changes

Rosejoycrocker force-pushed the separate-zone-predec-crit branch 2 times, most recently from 3e2f8c3 to 8fd18dc Compare February 6, 2024 05:10

Rosejoycrocker force-pushed the separate-zone-predec-crit branch from 8313957 to 61367b7 Compare February 9, 2024 04:39

Rosejoycrocker force-pushed the separate-zone-predec-crit branch 3 times, most recently from 098c952 to 7829620 Compare February 12, 2024 22:24

Rosejoycrocker and others added 24 commits February 16, 2024 08:05

Remove factors as input and replace with factors from input scenario …

047021d

…dataframe

Remove test of outcome_map (currently uses a function which no long…

3ca391d

…er exists, to be updated shortly in another PR)

Merge pull request #696 from open-AIMS/fix-rsa-test-error

b716025

Remove `factors` input from `rsa()` and use scenario dataframe column names instead

Make predec a vector input for consistency

3a34a30

Move calculation of zones and priority predecessor criteria into sepa…

4303a0e

…rate functions

Changes to avoid id mismatches when using new priority zones or prior…

ec3609a

…ity locations functions `strong_pred` and `zones` must be the same size

Change how strong_pred is input to guided_location_selection

18f9135

Fix and simplify zone criteria calculation

fff57c4

Zones criteria was incorrectly indexing using `strong_pred` - should have been using sites in `strong_pred` which contain priority zone sites, not values of `strong_pred` at sites in priority zones

Change references to "sites" to "locations" in docs for `priority_zon…

73e2aa3

…es_criteria`

Fix bug in how strong_pred is calculated

aac08d5

`strong_pred` should be a matrix, with site ids in the first column. Without this, calcs in priority predecessor and zones criteria are incorrect

Add weighting to zones criteria to vary how much predecessors vs bein…

368cb90

…g in a zone matters

Add location ids as input instead of changing size of strong_pred

48a5be4

Fix dimension mismatches and add to documentation

39964f0

Simplify and fix how `strong_pred` is used in zoning criteria Add to docs

Simplify formulation of priority predecessor criteria and add docs

d188eda

Remove unnecessary findall Remove unnecessary findall

Fix how weight is added if a location sites within a zone (should be …

b6f59f7

…additive, not replacement)

Update documentation in priority_zones_criteria

4d618ee

Add tests for priority predecessor and zones criteria + update decisi…

c2b8594

…on matrix tests

Change default weighting for zone criteria to equal for strongest sou…

0117656

…rces and zone members

Fix test - should test that sites which are both in and sources to a …

ecb6bbb

…priority zone have the highest criteria value

Update mcda tests due to change in number of inputs for seed matric c…

c495be0

…reation + some formatting

Fix typo in priority_zone_criteria function docs and format functio…

1f3307d

…n docs Fix formatting and style

Fix test for cover as was checking the wrong column

0acf954

Was checking column 6 which is priority predecessors, instead of 8 which is leftover space

Fix typo

3f4bb59

Formatting

Remove filtering as new normalisation means test for max cover may no…

dcc09c2

…t work

Rosejoycrocker force-pushed the separate-zone-predec-crit branch from 7829620 to dcc09c2 Compare February 20, 2024 05:54

ConnectedSystems changed the base branch from main to mcda-restructure February 21, 2024 07:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes, compartmentalisation and documentation for priority predecessor and zones criteria #634

Bug fixes, compartmentalisation and documentation for priority predecessor and zones criteria #634

Rosejoycrocker commented Dec 19, 2023

ConnectedSystems commented Jan 11, 2024

Rosejoycrocker commented Jan 11, 2024

ConnectedSystems commented Jan 11, 2024

ConnectedSystems commented Jan 11, 2024

Rosejoycrocker commented Jan 11, 2024 •

edited

Loading

ConnectedSystems Jan 23, 2024

ConnectedSystems Jan 23, 2024

ConnectedSystems Jan 23, 2024

ConnectedSystems Jan 23, 2024

ConnectedSystems commented Feb 9, 2024

Rosejoycrocker commented Feb 9, 2024

ConnectedSystems commented Feb 12, 2024

Rosejoycrocker commented Feb 13, 2024 •

edited

Loading

ConnectedSystems commented Feb 13, 2024 •

edited

Loading

Rosejoycrocker commented Feb 13, 2024

ConnectedSystems commented Feb 21, 2024

	risk> risk_tol are filtered out.
	risk > risk_tol are filtered out.

	strong_pred = zeros(Int64, (size(C1)...))
	strong_pred = zeros(Int64, size(C1))

Bug fixes, compartmentalisation and documentation for priority predecessor and zones criteria #634

Are you sure you want to change the base?

Bug fixes, compartmentalisation and documentation for priority predecessor and zones criteria #634

Conversation

Rosejoycrocker commented Dec 19, 2023

ConnectedSystems commented Jan 11, 2024

Rosejoycrocker commented Jan 11, 2024

ConnectedSystems commented Jan 11, 2024

ConnectedSystems commented Jan 11, 2024

Rosejoycrocker commented Jan 11, 2024 • edited Loading

ConnectedSystems Jan 23, 2024

Choose a reason for hiding this comment

ConnectedSystems Jan 23, 2024

Choose a reason for hiding this comment

ConnectedSystems Jan 23, 2024

Choose a reason for hiding this comment

ConnectedSystems Jan 23, 2024

Choose a reason for hiding this comment

ConnectedSystems commented Feb 9, 2024

Rosejoycrocker commented Feb 9, 2024

ConnectedSystems commented Feb 12, 2024

Rosejoycrocker commented Feb 13, 2024 • edited Loading

ConnectedSystems commented Feb 13, 2024 • edited Loading

Rosejoycrocker commented Feb 13, 2024

ConnectedSystems commented Feb 21, 2024

Rosejoycrocker commented Jan 11, 2024 •

edited

Loading

Rosejoycrocker commented Feb 13, 2024 •

edited

Loading

ConnectedSystems commented Feb 13, 2024 •

edited

Loading