FIBERALLOC-64: Charge the assignment arc cost to the objective only once

[monodera]

Summary

tcost accumulates vis_cost, cobraMoveCost and blackDotPenalty in turn, but f*tcost was added to the objective both before and after the black-dot term, so the visibility and cobra-move contributions were counted twice. An arc cost 2*(vis_cost + cobraMoveCost) + blackDotPenalty instead of (vis_cost + cobraMoveCost) + blackDotPenalty.

History

The duplicate dates back to dd6695f ("first iteration for black spot proximity penalty function"), which introduced the black-spot penalty along with a second accumulation rather than moving the existing one. Before that commit there was exactly one accumulation, so this is a copy-paste leftover rather than deliberate weighting.

Impact

Affects every backend (Gurobi, HiGHS, PuLP). The visibility and cobra-move terms carried double their configured weight relative to nonObservationCost, partialObservationCost and blackDotPenalty, so the optimizer traded them off differently than the configuration intends.

Note for review

This changes results wherever vis_cost or cobraMoveCost is nonzero. Correcting it effectively halves those terms relative to the other costs, and fiber assignments will shift. If the cost functions were tuned empirically against the current behaviour, we may prefer to rescale them alongside this fix rather than take the correction on its own.

Testing

python -m py_compile passes, and the single remaining accumulation now runs after all three cost terms are summed into tcost. The solver was not run end to end, so the effect on actual fiber assignments is unverified here and needs scientific validation.

Refs FIBERALLOC-64

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes an objective-function accounting bug in buildProblem() where the assignment arc cost was added to the optimization objective twice, causing vis_cost and cobraMoveCost to be double-counted relative to other cost terms.

Changes:

• Removes the earlier prob.cost += f*tcost so the arc cost is charged only once.
• Ensures the single remaining objective accumulation occurs after vis_cost, cobraMoveCost, and blackDotPenalty have all been summed into tcost.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mreineck]

Ouch, that was a really stupid copy/paste mistake on my part!
Since this affects results in potentially many cases, I'm not sure how we should proceed with merging? I can bring this up in the ICS/PFI+MCS telecon ... do you think this is enough?

[monodera]

I'm not sure how large the impact is. In addition to the ICS/PFI+MCS telecon (I'm not a part of it), you can also tell people in the pfs-netflow-users channel on Slack about it? I guess that the SSP WG representatives are more concerned about the change.

[mreineck]

Good point, I'll do that!
I will also make an entry in the ChangeLog referring to this (also for FIBERALLOC-63).

[mreineck]

I'm perfectly happy with the change!

Given the lack of objections on Slack, I think this can now be merged.