# mattwatts/marxan244

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.

# CalcPenalties Behaviour #2

Open
opened this Issue Mar 4, 2016 · 0 comments

Projects
None yet
1 participant

### mstrimas commented Mar 4, 2016

Thanks for putting the Marxan source code up on GitHub! I've been reading through it for fun and came across some odd behaviour in the CalcPenalties function when determining the base penalty for the target shortfall. I suspect this doesn't matter since it only results in a small change in base penalty, but I thought I'd mention it anyway. I put up a simple example demonstrating the behaviour on GitHub. Apologies for the long-windedness, I realize a lot of this is just me talking through stuff that is probably extremely obvious to you.

I'll focus on species 1 in the example. Here are the top 10 planning units for species 1 sorted by conservation value / cost. BLM = 0 and the target for this species is 272.

species pu amount cost status amount_per_cost
1 86 90 100 0 0.9000000
1 7 70 100 0 0.7000000
1 25 70 100 0 0.7000000
1 98 50 100 0 0.5000000
1 37 70 200 0 0.3500000
1 5 90 300 0 0.3000000
1 67 90 400 0 0.2250000
1 58 60 400 0 0.1500000
1 80 80 600 0 0.1333333
1 16 100 800 0 0.1250000

CalcPenalties chooses units 86, 7, 25, and 5 for a cost of 600. Just taking the most efficient (w.r.t. amount/cost) planning units would lead to 98 being chosen instead of 5, for a cost of 400. The reason is this bit of code (lines 1287-99 in MarOpt_v244.c):

```if (rAmount >= spec[i].target - ftarget &&
(imaxtarget == 0 || (imaxtarget == 1 && fcost < fbest)))
{
imaxtarget = 1;
ibest = j;
fbest = fcost;
} // can I meet the target cheaply?
else
if (fbestrat < rAmount/fcost)
{
fbest = fcost;
fbestrat = rAmount/fbest;
ibest = j;
}  // finding the cheapest planning unit```

My understanding of this: The second part of the if statement just choses the PU with highest amount/cost. However, it's possible just adding PUs in order of amount/cost isn't the most efficient. If, at any point, there's a single PU that, when added, would meet the target, it would be better to pick this provided it has lower cost that the PU with highest amount/cost. The first part is meant to address this case.

However, I don't think this if statement is doing what's intended. For example, after PUs 86, 7, and 25 are added, only 42 more units of representation are needed to meet the target of 272. Now, many of the PUs will meet the target. For example, PU 37 will achieve the target for only 200 units of cost (the cheapest), however, after `ibest` and `fcost` are are set to this PU they are lost when, later in the loop, PUs with lower amount/cost are encountered. The reason PU 5 is eventually chosen is because it's the last PU encountered that meets the target (the loop goes backwards from PU 100 to PU 1 for reasons I don't understand), not because it's cheapest.

Wondering if it would be better the decouple the if-else into two separate if statements that independently find the PU with lowest amount/value and the PU with lost cost that meets the target. Then in a final step comparing the two bests to see which is cheaper. As it stands, I think the function will almost always end up with higher cost than would be achieved if the PUs were just added in order of amount / cost.

```if (rAmount >= spec[i].target - ftarget &&
(imaxtarget == 0 || (imaxtarget == 1 && fcost < fbestsingle)))
{
imaxtarget = 1;
ibestsingle = j;
fbestsingle = fcost;
} // can I meet the target cheaply?

if (fbestrat < rAmount/fcost)
{
fbest = fcost;
fbestrat = rAmount/fbest;
ibest = j;
}  // finding the cheapest planning unit
...
...
...
//after loop ends
if (fbestsingle < fbest) {
fbest = fbestsingle
ibest = ibestsingle
}```
to join this conversation on GitHub. Already have an account? Sign in to comment