Merge pull request #902 from EnergySystemsModellingLab/gate-broken-model-options

Scare users away from using broken options

Author: alexdewar

schemas/input/agent_objectives.yaml

@@ -23,7 +23,8 @@ fields:
     description: The type of objective
     notes: |
       Must be `npv` (net present value) or `lcox` (levelised cost of X). Note that support for NPV
-      is experimental and may give bad results.
+      is [currently broken](https://github.com/EnergySystemsModellingLab/MUSE2/issues/716), so don't
+      enable this option unless you know what you're doing.
   - name: decision_weight
     type: number
     description: Weight for weighted sum decision rule

schemas/input/model.yaml

@@ -47,5 +47,9 @@ properties:
     type: number
     description: The relative tolerance for price convergence in the ironing out loop
     default: 1e-6
+  please_give_me_broken_results:
+    type: boolean
+    description: |
+      Allows other options that are known to be broken to be used. Please don't ever enable this.
 
 required: [milestone_years]

src/input/agent/objective.rs

@@ -1,6 +1,8 @@
 //! Code for reading the agent objectives CSV file.
 use super::super::{input_err_msg, read_csv, try_insert};
+use crate::ISSUES_URL;
 use crate::agent::{AgentID, AgentMap, AgentObjectiveMap, DecisionRule, ObjectiveType};
+use crate::model::{ALLOW_BROKEN_OPTION_NAME, broken_model_options_allowed};
 use crate::units::Dimensionless;
 use crate::year::parse_year_str;
 use anyhow::{Context, Result, ensure};
@@ -32,6 +34,8 @@ struct AgentObjectiveRaw {
 /// # Arguments
 ///
 /// * `model_dir` - Folder containing model configuration files
+/// * `agents` - Map of agents
+/// * `milestone_years` - Milestone years for the simulation
 ///
 /// # Returns
 ///
@@ -96,9 +100,15 @@ where
             .filter(|year| agent_objectives[year] == ObjectiveType::NetPresentValue)
             .collect_vec();
         if !npv_years.is_empty() {
+            ensure!(
+                broken_model_options_allowed(),
+                "The NPV option is BROKEN and should not be used. See: {ISSUES_URL}/716.\n\
+                If you are sure that you want to enable it anyway, you need to set the \
+                {ALLOW_BROKEN_OPTION_NAME} option to true."
+            );
             warn!(
                 "Agent {agent_id} is using NPV in years {npv_years:?}. \
-                Support for NPV is currently experimental and may give bad results."
+                The NPV option is BROKEN and should not be used. See: {ISSUES_URL}/716."
             );
         }
     }

src/lib.rs

@@ -1,6 +1,9 @@
 //! Common functionality for MUSE2.
 #![warn(missing_docs)]
 
+/// The main GitHub issues page for MUSE2
+pub const ISSUES_URL: &str = concat!(env!("CARGO_PKG_REPOSITORY"), "/issues");
+
 use dirs::config_dir;
 use std::path::PathBuf;
 

src/main.rs

@@ -1,12 +1,12 @@
 use human_panic::{metadata, setup_panic};
 use log::error;
+use muse2::ISSUES_URL;
 use muse2::cli::run_cli;
 use muse2::log::is_logger_initialised;
 
 fn main() {
     setup_panic!(metadata!().support(format!(
-        "Open an issue on Github: {}/issues/new?template=bug_report.md",
-        env!("CARGO_PKG_REPOSITORY")
+        "Open an issue on Github: {ISSUES_URL}/new?template=bug_report.md"
     )));
 
     if let Err(err) = run_cli() {

src/model.rs

@@ -8,7 +8,9 @@ use std::collections::HashMap;
 use std::path::PathBuf;
 
 pub mod parameters;
-pub use parameters::{ModelParameters, PricingStrategy};
+pub use parameters::{
+    ALLOW_BROKEN_OPTION_NAME, ModelParameters, PricingStrategy, broken_model_options_allowed,
+};
 
 /// Model definition
 pub struct Model {

src/model/parameters.rs

@@ -1,4 +1,5 @@
 //! Defines the `ModelParameters` struct, which represents the contents of `model.toml`.
+use crate::ISSUES_URL;
 use crate::asset::check_capacity_valid_for_asset;
 use crate::input::{
     deserialise_proportion_nonzero, input_err_msg, is_sorted_and_unique, read_toml,
@@ -9,9 +10,23 @@ use log::warn;
 use serde::Deserialize;
 use serde_string_enum::DeserializeLabeledStringEnum;
 use std::path::Path;
+use std::sync::OnceLock;
 
 const MODEL_PARAMETERS_FILE_NAME: &str = "model.toml";
 
+/// The name of the option used to gate other, broken options.
+pub const ALLOW_BROKEN_OPTION_NAME: &str = "please_give_me_broken_results";
+
+/// Whether broken options have been enabled by an option in the model config file
+static BROKEN_OPTIONS_ALLOWED: OnceLock<bool> = OnceLock::new();
+
+/// Whether broken model options have been enabled in the config file or not
+pub fn broken_model_options_allowed() -> bool {
+    *BROKEN_OPTIONS_ALLOWED
+        .get()
+        .expect("Broken options flag not set")
+}
+
 macro_rules! define_unit_param_default {
     ($name:ident, $type: ty, $value: expr) => {
         fn $name() -> $type {
@@ -42,6 +57,9 @@ define_param_default!(default_max_ironing_out_iterations, u32, 10);
 pub struct ModelParameters {
     /// Milestone years
     pub milestone_years: Vec<u32>,
+    /// Allow known-broken options to be enabled.
+    #[serde(default, rename = "please_give_me_broken_results")] // Can't use constant here :-(
+    pub allow_broken_options: bool,
     /// The (small) value of capacity given to candidate assets.
     ///
     /// Don't change unless you know what you're doing.
@@ -134,6 +152,11 @@ impl ModelParameters {
         let file_path = model_dir.as_ref().join(MODEL_PARAMETERS_FILE_NAME);
         let model_params: ModelParameters = read_toml(&file_path)?;
 
+        // Set flag signalling whether broken model options are allowed or not
+        BROKEN_OPTIONS_ALLOWED
+            .set(model_params.allow_broken_options)
+            .unwrap(); // Will only fail if there is a race condition, which shouldn't happen
+
         model_params
             .validate()
             .with_context(|| input_err_msg(file_path))?;
@@ -143,15 +166,30 @@ impl ModelParameters {
 
     /// Validate parameters after reading in file
     fn validate(&self) -> Result<()> {
+        if self.allow_broken_options {
+            warn!(
+                "!!! You've enabled the {ALLOW_BROKEN_OPTION_NAME} option. !!!\n\
+                I see you like to live dangerously 😈. This option should ONLY be used by \
+                developers as it can cause peculiar behaviour that breaks things. NEVER enable it \
+                for results you actually care about or want to publish. You have been warned!"
+            );
+        }
+
         // milestone_years
         check_milestone_years(&self.milestone_years)?;
 
         // pricing_strategy
         if self.pricing_strategy == PricingStrategy::ScarcityAdjusted {
+            ensure!(
+                self.allow_broken_options,
+                "The pricing strategy is set to 'scarcity_adjusted', which is known to be broken. \
+                If you are sure that you want to enable it anyway, you need to set the \
+                {ALLOW_BROKEN_OPTION_NAME} option to true."
+            );
+
             warn!(
                 "The pricing strategy is set to 'scarcity_adjusted'. Commodity prices may be \
-                incorrect if assets have more than one output commodity. See: {}/issues/677",
-                env!("CARGO_PKG_REPOSITORY")
+                incorrect if assets have more than one output commodity. See: {ISSUES_URL}/677"
             );
         }