refactor and remove capacity term from npv objective

[Aurashk]

Description

A slight refactoring so that that the lcox and npv optimisations can have different variables, then removes the annualised fixed cost from the npv maximisation

Fixes #996

see #998 for a results comparison with simple model

Type of change

• Bug fix (non-breaking change to fix an issue)
• New feature (non-breaking change to add functionality)
• Refactoring (non-breaking, non-functional change to improve maintainability)
• Optimization (non-breaking change to speed up the code)
• Breaking change (whatever its nature)
• Documentation (improve or add documentation)

Key checklist

• All tests pass: $ cargo test
• The documentation builds and looks OK: $ cargo doc

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.28%. Comparing base (8c8509f) to head (3c96831).
⚠️ Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/investment/appraisal.rs	0.00%	1 Missing ⚠️
...rc/simulation/investment/appraisal/coefficients.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
+ Coverage   84.26%   84.28%   +0.01%     
==========================================
  Files          52       52              
  Lines        5974     5974              
  Branches     5974     5974              
==========================================
+ Hits         5034     5035       +1     
+ Misses        695      694       -1     
  Partials      245      245              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dalonsoa]

The solution seems OK to me, but it can be implemented with much less code duplication, and less changes, I would say,

[dalonsoa]

Actually, the simplest solution I think would be to add objective_type to the ObjectiveCoefficients struct, such that whenever that struct is required, the objective type can be query in case any specific customisation needs to be in place, like in here. If you do that, then you don't need to change the arguments of any function, which is nice and less arbitrary.

[tsmbland]

I think these are all valid options. Just to confuse things even more I'll give another option:

Going back to how it was before, you could modify calculate_coefficients_for_npv so the capacity coefficient stored in ObjectiveCoefficients is zero. Then you'd modify calculate_npv so that, rather taking annual_fixed_cost from ObjectiveCoefficients (this was just a shortcut because we happen to have already calculated the thing we need), you could calculate the annual fixed cost there and then (annual_fixed_cost(asset)).

I'd probably lean towards this so that ObjectiveCoefficients is doing what it says and storing the actual coefficients we use for the optimisation

[dalonsoa]

I agree with @tsmbland 's solution. If a non-zero capacity coefficient is not really needed in ObjectiveCoefficients, let's make it zero to start with.

[Aurashk]

Thanks, suggestions make sense.

Just wanted to raise one other thing - I've temporarily added #[allow(clippy::too_many_arguments)] to perform_optimisation but it seems like the general advice is to combine arguments into a struct once you cross a certain threshold. Does it make sense to have an OptimisationParameters struct in this case which and have that as the only argument to perform_optimisation?

Also I'll add something to the docs about zeroing the term in npv mode

[tsmbland]

Thanks, suggestions make sense.

Just wanted to raise one other thing - I've temporarily added #[allow(clippy::too_many_arguments)] to perform_optimisation but it seems like the general advice is to combine arguments into a struct once you cross a certain threshold. Does it make sense to have an OptimisationParameters struct in this case which and have that as the only argument to perform_optimisation?

I'm guilty of doing #[allow(clippy::too_many_arguments)] a lot, and personally I don't think it's such a big deal and wouldn't necessarily be any cleaner making a struct.

But in this case, if you went for my suggestion above, I don't think you'd need to do this, right? (perform_optimisation would go back to how it was)

[Aurashk]

Thanks, suggestions make sense.
Just wanted to raise one other thing - I've temporarily added #[allow(clippy::too_many_arguments)] to perform_optimisation but it seems like the general advice is to combine arguments into a struct once you cross a certain threshold. Does it make sense to have an OptimisationParameters struct in this case which and have that as the only argument to perform_optimisation?

I'm guilty of doing #[allow(clippy::too_many_arguments)] a lot, and personally I don't think it's such a big deal and wouldn't necessarily be any cleaner making a struct.

But in this case, if you went for my suggestion above, I don't think you'd need to do this, right? (perform_optimisation would go back to how it was)

ah yes that's a good point thanks. I forgot it had one less variable

[Aurashk]

Thanks for the comments, I agree simpler this way. Although I'm wondering if we should remove add_variables and make it a constructor to VariableMap instead. I feel like it caused me some confusion because add_constraints only modifies existing data structures, but add_variables is responsible for making VariableMap's

so would replace

fn add_variables_to_problem(
    problem: &mut Problem,
    cost_coefficients: &ObjectiveCoefficients,
) -> VariableMap {
    // Create capacity variable
    let capacity_var = problem.add_column(cost_coefficients.capacity_coefficient.value(), 0.0..);

    // Create activity variables
    let mut activity_vars = IndexMap::new();
    for (time_slice, cost) in &cost_coefficients.activity_coefficients {
        let var = problem.add_column(cost.value(), 0.0..);
        activity_vars.insert(time_slice.clone(), var);
    }

    // Create unmet demand variables
    // One per time slice, all of which use the same coefficient
    let mut unmet_demand_vars = IndexMap::new();
    for time_slice in cost_coefficients.activity_coefficients.keys() {
        let var = problem.add_column(cost_coefficients.unmet_demand_coefficient.value(), 0.0..);
        unmet_demand_vars.insert(time_slice.clone(), var);
    }

    VariableMap {
        capacity_var,
        activity_vars,
        unmet_demand_vars,
    }
}

with

impl VariableMap {
    /// Creates a new variable map by adding variables to the optimisation problem.
    ///
    /// # Arguments
    /// * `problem` - The optimisation problem to add variables to
    /// * `cost_coefficients` - Objective function coefficients for each variable
    ///
    /// # Returns
    /// A new `VariableMap` containing all created decision variables
    fn new(
        problem: &mut Problem,
        cost_coefficients: &ObjectiveCoefficients,
    ) -> Self {
        // Create capacity variable with its associated cost
        let capacity_var = problem.add_column(
            cost_coefficients.capacity_coefficient.value(),
            0.0..,
        );

        // Create activity variables for each time slice
        let mut activity_vars = IndexMap::new();
        for (time_slice, cost) in &cost_coefficients.activity_coefficients {
            let var = problem.add_column(cost.value(), 0.0..);
            activity_vars.insert(time_slice.clone(), var);
        }

        // Create unmet demand variables for each time slice
        let mut unmet_demand_vars = IndexMap::new();
        for time_slice in cost_coefficients.activity_coefficients.keys() {
            let var = problem.add_column(
                cost_coefficients.unmet_demand_coefficient.value(),
                0.0..,
            );
            unmet_demand_vars.insert(time_slice.clone(), var);
        }

        Self {
            capacity_var,
            activity_vars,
            unmet_demand_vars,
        }
    }
}

On that same thread I would also consider making calculate_coefficients_for_lcox/npv two different constructors for ObjectiveCoefficients instead of standalone functions since they are similarly taking some inputs and giving you back an ObjectiveCoefficients instance.

Appreciate this might be outside of the scope of the PR though!

[tsmbland]

impl VariableMap {
    /// Creates a new variable map by adding variables to the optimisation problem.
    ///
    /// # Arguments
    /// * `problem` - The optimisation problem to add variables to
    /// * `cost_coefficients` - Objective function coefficients for each variable
    ///
    /// # Returns
    /// A new `VariableMap` containing all created decision variables
    fn new(
        problem: &mut Problem,
        cost_coefficients: &ObjectiveCoefficients,
    ) -> Self {
        // Create capacity variable with its associated cost
        let capacity_var = problem.add_column(
            cost_coefficients.capacity_coefficient.value(),
            0.0..,
        );

        // Create activity variables for each time slice
        let mut activity_vars = IndexMap::new();
        for (time_slice, cost) in &cost_coefficients.activity_coefficients {
            let var = problem.add_column(cost.value(), 0.0..);
            activity_vars.insert(time_slice.clone(), var);
        }

        // Create unmet demand variables for each time slice
        let mut unmet_demand_vars = IndexMap::new();
        for time_slice in cost_coefficients.activity_coefficients.keys() {
            let var = problem.add_column(
                cost_coefficients.unmet_demand_coefficient.value(),
                0.0..,
            );
            unmet_demand_vars.insert(time_slice.clone(), var);
        }

        Self {
            capacity_var,
            activity_vars,
            unmet_demand_vars,
        }
    }
}

I like this. I'd maybe rename it to add_to_problem (i.e. x = VariableMap::add_to_problem(problem, cost_coefficients)) just so it's a bit more descriptive of what it's doing

[dalonsoa]

I like all of these suggestions. They surely make the code clerer.

On that same thread I would also consider making calculate_coefficients_for_lcox/npv two different constructors for ObjectiveCoefficients instead of standalone functions since they are similarly taking some inputs and giving you back an ObjectiveCoefficients instance.

And this one I would leave for a separate PR, yes.

[tsmbland]

Looks good!

[dalonsoa]

LGTM! Much clearer!

[tsmbland]

@Aurashk I'm going to merge this as I want to try it with #999