Error when using other MCDA rankings

[ConnectedSystems]

Hi @Rosejoycrocker

I was thinking through what tests are needed to cover the codebase and in my travels ran into the errors below (using my unify-runADRIA branch).

Please note that the runScenarios will replace runADRIA. More specifically, I plan to delete the current runADRIA and rename runScenario to runADRIA.

Could we discuss sometime next week? I haven't looked too deeply into this but my initial guess is there's been an incompatible change in runADRIAScenario

For alg_ind = 2

Arrays have incompatible sizes for this operation.

Error in ADRIA_DMCDA (line 121)
        SE = SE.* repmat(wse,nsites,1);

Error in runADRIAScenario (line 178)
            [prefseedsites, prefshadesites, nprefseedsites, nprefshadesites] = ADRIA_DMCDA(dMCDA_vars, alg_ind); % site selection function for intervention deployment

Error in runScenarios (line 89)
        Y(i, j) = runADRIAScenario(scen_it, scen_crit, ...

Error in run_ADRIA_example (line 77)
Y = runScenarios(IT, criteria_weights, param_tbl, ecol_tbl, ...

For alg_ind = 3:

Arrays have incompatible sizes for this operation.

Error in ADRIA_DMCDA (line 204)
        SE = SE.* repmat(wse,nsites,1);

Error in runADRIAScenario (line 178)
            [prefseedsites, prefshadesites, nprefseedsites, nprefshadesites] = ADRIA_DMCDA(dMCDA_vars, alg_ind); % site selection function for intervention deployment

Error in runScenarios (line 89)
        Y(i, j) = runADRIAScenario(scen_it, scen_crit, ...

Error in run_ADRIA_example (line 77)
Y = runScenarios(IT, criteria_weights, param_tbl, ecol_tbl, ...

[Rosejoycrocker]

Hi @ConnectedSystems Yeah looks like something to do with the single scenario runs. I'll have a look into it so we can chat about it next week.

[ConnectedSystems]

Making a note before I forget.

For alg_ind = 2

SE(:,1) = A(:,1); %sites column (remaining)

% ... snip ...

SE = SE.* repmat(wse,nsites,1);

wse is repeated nsites times but SE may hold a lesser number of rows as sites get filtered out (seems this is what variable A relates to), so replacing use of nsites with height(A) may be the fix.

[ConnectedSystems]

Hi @Rosejoycrocker

Seems I spoke too soon on #37 - alg_ind = 2 seems to work but I run into an error with alg_ind = 3 on L 214 in ADRIA_DMCA.m

S = [sites', S];

Error using ADRIA_DMCDA (line 214)
Dimensions of arrays being concatenated are not consistent.

Error in runADRIAScenario (line 178)
            [prefseedsites, prefshadesites, nprefseedsites, nprefshadesites] = ADRIA_DMCDA(dMCDA_vars, alg_ind); % site selection function for intervention deployment

Error in runScenarios (line 78)
parfor i = 1:N

Error in run_ADRIA_example (line 77)
Y = runScenarios(interv_scens, criteria_weights, param_tbl, ecol_tbl, ...

Suspect it is a mismatch in the number of sites or similar.

After you fix, could you write tests to catch these in case it pops up again?

[Rosejoycrocker]

Hi @ConnectedSystems Thanks for letting me know, I think I know what the issue is- when I was testing the DMCDA algorithms I must have used cases for which all sites were used. This error arises because theres a step where you pair sites against their rankings and so if some sites have been eliminated due to wave risk or heat stress thresholds the ranking and site vector sizes differ.

I'll fix this today and try and write some tests. Are you aiming to have these tests all in one script? (like have you got a script already that runs a series of tests? sorry if you've already shown me something like this).

[ConnectedSystems]

Are you aiming to have these tests all in one script?

Normally I'd write a single function for each aspect being tested, and group these by file (e.g., testLoadingSomeSpecificData.m)

But because MATLAB follows the one file = one function convention I've taken to writing these as a script for individual concerns with separate sections (indicated by '%%').

The most recent tests are in testSamplingConversion.m in the tests directory (the others are for deprecated functions and so are now redundant).

Note that I am doing a Very Bad Thing™, as I do not have adequate documentation on what the tests are doing and the asserts are "blind" - there is no associated message indicating what went wrong to whoever is running the tests.

https://au.mathworks.com/help/matlab/ref/assert.html

I definitely did not follow good practice there, my apologies.

[Rosejoycrocker]

Thanks, I'll have a look at that script. The problem is fixed now ( I think I actually fixed it in algorithm 2 previously and forgot to transfer similar line changes to algorithm's 3 and 4. But I'll go and write a test script now.

[ConnectedSystems]

like have you got a script already that runs a series of tests?

Sorry, I missed this question:

runtests('tests')

where 'tests' refers to the tests directory. This will run all the tests in the test folder (only works after you've added the folder to the project path).

This is somewhat documented in https://github.com/open-AIMS/ADRIA_repo/blob/main/docs/dev_setup.md#tests but the approach there involves changing directories which may cause confusion/issues for legacy code that relies on the current location.

[Rosejoycrocker]

Thanks for that :) I'm not actually sure how to write a test for the bug I just fixed as its not to do with inputs but how the code itself was written (i.e. the test would have to sit inside the MCDA function). The error should not turn up again because the code has been rewritten to be flexible to a changing number of sites to select from.

[ConnectedSystems]

how to write a test for the bug I just fixed as its not to do with inputs but how the code itself was written

Yeah, so the test would check that a hypothetical future change doesn't inadvertently introduce a similar issue. A drastic example is that there's an unrelated incorrect change that needs to be reverted, but in rolling back the problematic code, I stuff up the merge and accidentally replace this version with the older version.

Another example is if someone makes a change that accidentally means the number of sites are static On this one, the broad test I mocked up only indicates no errors are raised if different numbers of sites are used. It's limited though because it relies on the DHW dataset which assumes 26 sites - not sure how to mock up a varying number of sites.

i.e. the test would have to sit inside the MCDA function

This may signify that the function needs to be split into smaller functions so that you can target tests. In general, small functions that allow you to compose behavior is generally better (and more testable). SRP and all that.

There's some minor duplication of code in ADRIA_DMCDA around the places where the issue popped up. I think you had to replace (near identical?) code in 3 separate locations - and this will grow as more methods are added.

This could mean that the near identical portions could be moved into a separate function which is then targeted for tests instead of the bigger ADRIA_DMCDA() function. Alternatively, these duplicates could be moved outside of the switch statement so you only have to change code in a single place if issues pop up in the future.

Some test ideas:

• Does it work with a single site?
• zero sites?
• Works when minimum number of sites are specified?
• Should it raise errors under some conditions?
• Beyond this specific issue, how can we ensure the MCDA methods are behaving properly? (e.g., are they making expected/sensible decisions?)

To emphasize, regression tests like these are to catch things that work as expected now, but would be disastrous if they stopped working as expected later, for whatever reason.

[Rosejoycrocker]

Right, so a general testing script for the MCDA, sorry I thought you meant just for that particular issue. I'll have a look at the MCDA testing function you write and see if I can add anything :)

[ConnectedSystems]

I thought you meant just for that particular issue

I did initially, but then I veered off :)

[ConnectedSystems]

Closed by #39