From a5fb37cb2abc08756b97537f0aeed3ba052c9fff Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Tue, 24 Mar 2026 16:06:43 +0000 Subject: [PATCH] Change variable names and comments to talk about "dangerous" cf. "broken" options In various places in the code and documentation we talk about broken options being gated by the `please_give_me_broken_results` parameter. Strictly speaking, it is not true that all these hidden options are broken. For example, the `remaining_demand_absolute_tolerance` parameter is not broken: the reason for hiding it is that it is potentially dangerous for users to change it. Change variable names, comments etc. to talk about "dangerous" rather than "broken" options, where appropriate. I have left the name of the `please_give_me_broken_results` parameter the same, as enabling any of these options may lead to broken (or nonsensical) _results_, even if the option itself is not broken. This is mostly a developer-facing change, but I have also updated the language used in the schema. --- schemas/input/model.yaml | 4 +- src/input/commodity.rs | 6 +-- src/model.rs | 4 +- src/model/parameters.rs | 88 ++++++++++++++++++++-------------------- 4 files changed, 53 insertions(+), 49 deletions(-) diff --git a/schemas/input/model.yaml b/schemas/input/model.yaml index c907c36af..7b4f64d0b 100644 --- a/schemas/input/model.yaml +++ b/schemas/input/model.yaml @@ -49,7 +49,9 @@ properties: 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. + Allows other options that are potentially dangerous to be used, such as experimental features and parameters that are known to have correctness bugs. This option should only ever be enabled by developers. + + Never enable this for experiments that you care about, particularly if you intend to publish the results. remaining_demand_absolute_tolerance: type: number description: | diff --git a/src/input/commodity.rs b/src/input/commodity.rs index 96e0601ad..69b449049 100644 --- a/src/input/commodity.rs +++ b/src/input/commodity.rs @@ -5,7 +5,7 @@ use crate::commodity::{ BalanceType, Commodity, CommodityID, CommodityLevyMap, CommodityMap, CommodityType, DemandMap, PricingStrategy, }; -use crate::model::{ALLOW_BROKEN_OPTION_NAME, broken_model_options_allowed}; +use crate::model::{ALLOW_DANGEROUS_OPTION_NAME, dangerous_model_options_enabled}; use crate::region::RegionID; use crate::time_slice::{TimeSliceInfo, TimeSliceLevel}; use anyhow::{Context, Ok, Result, ensure}; @@ -167,9 +167,9 @@ fn validate_commodity(commodity: &Commodity) -> Result<()> { // Gatekeep scarcity-adjusted pricing option if commodity.pricing_strategy == PricingStrategy::ScarcityAdjusted { ensure!( - broken_model_options_allowed(), + dangerous_model_options_enabled(), "The 'scarcity' pricing strategy is currently experimental. \ - To run anyway, set the {ALLOW_BROKEN_OPTION_NAME} option to true." + To run anyway, set the {ALLOW_DANGEROUS_OPTION_NAME} option to true." ); warn!( "The pricing strategy for {} is set to 'scarcity'. Commodity prices may be \ diff --git a/src/model.rs b/src/model.rs index 789da3c89..913a5f9d7 100644 --- a/src/model.rs +++ b/src/model.rs @@ -10,7 +10,9 @@ use std::collections::HashMap; use std::path::PathBuf; pub mod parameters; -pub use parameters::{ALLOW_BROKEN_OPTION_NAME, ModelParameters, broken_model_options_allowed}; +pub use parameters::{ + ALLOW_DANGEROUS_OPTION_NAME, ModelParameters, dangerous_model_options_enabled, +}; /// Model definition pub struct Model { diff --git a/src/model/parameters.rs b/src/model/parameters.rs index dd1a2a725..9809ba407 100644 --- a/src/model/parameters.rs +++ b/src/model/parameters.rs @@ -1,8 +1,8 @@ //! Read and validate model parameters from `model.toml`. //! -//! This module defines the `ModelParameters` struct and helpers for loading and -//! validating the `model.toml` configuration used by the model. Validation -//! functions ensure sensible numeric ranges and invariants for runtime use. +//! This module defines the `ModelParameters` struct and helpers for loading and validating the +//! `model.toml` configuration used by the model. Validation functions ensure sensible numeric +//! ranges and invariants for runtime use. use crate::asset::check_capacity_valid_for_asset; use crate::input::{ deserialise_proportion_nonzero, input_err_msg, is_sorted_and_unique, read_toml, @@ -16,42 +16,42 @@ use std::sync::OnceLock; const MODEL_PARAMETERS_FILE_NAME: &str = "model.toml"; -/// The key in `model.toml` which enables known-broken model options. +/// The key in `model.toml` which enables potentially dangerous model options. /// -/// If this option is present and true, the model will permit certain -/// experimental or unsafe behaviours that are normally disallowed. -pub const ALLOW_BROKEN_OPTION_NAME: &str = "please_give_me_broken_results"; +/// If this option is present and true, the model will permit certain experimental or unsafe +/// behaviours that are normally disallowed. +pub const ALLOW_DANGEROUS_OPTION_NAME: &str = "please_give_me_broken_results"; -/// Global flag indicating whether broken model options have been enabled. +/// Global flag indicating whether potentially dangerous model options have been enabled. /// -/// This is stored in a `OnceLock` and must be set exactly once during -/// startup (see `set_broken_model_options_flag`). -static BROKEN_OPTIONS_ALLOWED: OnceLock = OnceLock::new(); +/// This is stored in a `OnceLock` and must be set exactly once during startup (see +/// [`set_dangerous_model_options_flag`]). +static DANGEROUS_OPTIONS_ENABLED: OnceLock = OnceLock::new(); -/// Return whether broken model options were enabled by the loaded config. +/// Whether potentially dangerous model options were enabled by the loaded config. /// /// # Panics /// /// Panics if the global flag has not been set yet (the flag should be set by -/// `ModelParameters::from_path` during program initialization). -pub fn broken_model_options_allowed() -> bool { - *BROKEN_OPTIONS_ALLOWED +/// [`ModelParameters::from_path`] during program initialisation). +pub fn dangerous_model_options_enabled() -> bool { + *DANGEROUS_OPTIONS_ENABLED .get() - .expect("Broken options flag not set") + .expect("Dangerous options flag not set") } -/// Set the global flag indicating whether broken model options are allowed. +/// Set the global flag indicating whether potentially dangerous model options are enabled. /// /// Can only be called once; subsequent calls will panic (except in tests, where it can be called /// multiple times so long as the value is the same). -fn set_broken_model_options_flag(allowed: bool) { - let result = BROKEN_OPTIONS_ALLOWED.set(allowed); +fn set_dangerous_model_options_flag(enabled: bool) { + let result = DANGEROUS_OPTIONS_ENABLED.set(enabled); if result.is_err() { if cfg!(test) { // Sanity check - assert_eq!(allowed, broken_model_options_allowed()); + assert_eq!(enabled, dangerous_model_options_enabled()); } else { - panic!("Attempted to set BROKEN_OPTIONS_ALLOWED twice"); + panic!("Attempted to set DANGEROUS_OPTIONS_ENABLED twice"); } } } @@ -89,9 +89,9 @@ define_param_default!(default_mothball_years, u32, 0); pub struct ModelParameters { /// Milestone years pub milestone_years: Vec, - /// Allow known-broken options to be enabled. + /// Allow potentially dangerous options to be enabled. #[serde(default, rename = "please_give_me_broken_results")] // Can't use constant here :-( - pub allow_broken_options: bool, + pub allow_dangerous_options: bool, /// The (small) value of capacity given to candidate assets. /// /// Don't change unless you know what you're doing. @@ -169,7 +169,7 @@ fn check_price_tolerance(value: Dimensionless) -> Result<()> { } fn check_remaining_demand_absolute_tolerance( - allow_broken_options: bool, + dangerous_options_enabled: bool, value: Flow, ) -> Result<()> { ensure!( @@ -178,12 +178,12 @@ fn check_remaining_demand_absolute_tolerance( ); let default_value = default_remaining_demand_absolute_tolerance(); - if !allow_broken_options { + if !dangerous_options_enabled { ensure!( value == default_value, - "Setting a remaining_demand_absolute_tolerance different from the default value of {:e} \ - is potentially dangerous, set please_give_me_broken_results to true \ - if you want to allow this.", + "Setting a remaining_demand_absolute_tolerance different from the default value of \ + {:e} is potentially dangerous, set {ALLOW_DANGEROUS_OPTION_NAME} to true if you want \ + to allow this.", default_value.0 ); } @@ -215,7 +215,7 @@ impl ModelParameters { let file_path = model_dir.as_ref().join(MODEL_PARAMETERS_FILE_NAME); let model_params: ModelParameters = read_toml(&file_path)?; - set_broken_model_options_flag(model_params.allow_broken_options); + set_dangerous_model_options_flag(model_params.allow_dangerous_options); model_params .validate() @@ -226,9 +226,9 @@ impl ModelParameters { /// Validate parameters after reading in file fn validate(&self) -> Result<()> { - if self.allow_broken_options { + if self.allow_dangerous_options { warn!( - "!!! You've enabled the {ALLOW_BROKEN_OPTION_NAME} option. !!!\n\ + "!!! You've enabled the {ALLOW_DANGEROUS_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!" @@ -258,7 +258,7 @@ impl ModelParameters { // remaining_demand_absolute_tolerance check_remaining_demand_absolute_tolerance( - self.allow_broken_options, + self.allow_dangerous_options, self.remaining_demand_absolute_tolerance, )?; @@ -390,12 +390,12 @@ mod tests { } #[rstest] - #[case(true, 0.0, true)] // Valid minimum value broken options allowed - #[case(true, 1e-10, true)] // Valid value with broken options allowed - #[case(true, 1e-15, true)] // Valid value with broken options allowed - #[case(false, 1e-12, true)] // Valid value same as default, no broken options needed - #[case(true, 1.0, true)] // Valid larger value with broken options allowed - #[case(true, f64::MAX, true)] // Valid maximum finite value with broken options allowed + #[case(true, 0.0, true)] // Valid minimum value dangerous options allowed + #[case(true, 1e-10, true)] // Valid value with dangerous options allowed + #[case(true, 1e-15, true)] // Valid value with dangerous options allowed + #[case(false, 1e-12, true)] // Valid value same as default, no dangerous options needed + #[case(true, 1.0, true)] // Valid larger value with dangerous options allowed + #[case(true, f64::MAX, true)] // Valid maximum finite value with dangerous options allowed #[case(true, -1e-10, false)] // Invalid: negative value #[case(true, f64::INFINITY, false)] // Invalid: positive infinity #[case(true, f64::NEG_INFINITY, false)] // Invalid: negative infinity @@ -405,12 +405,12 @@ mod tests { #[case(false, f64::NEG_INFINITY, false)] // Invalid: negative infinity #[case(false, f64::NAN, false)] // Invalid: NaN fn check_remaining_demand_absolute_tolerance_works( - #[case] allow_broken_options: bool, + #[case] allow_dangerous_options: bool, #[case] value: f64, #[case] expected_valid: bool, ) { let flow = Flow::new(value); - let result = check_remaining_demand_absolute_tolerance(allow_broken_options, flow); + let result = check_remaining_demand_absolute_tolerance(allow_dangerous_options, flow); assert_validation_result( result, @@ -425,7 +425,7 @@ mod tests { #[case(1e-10)] // Larger than default (1e-12) #[case(1.0)] // Well above default #[case(f64::MAX)] // Maximum finite value - fn check_remaining_demand_absolute_tolerance_requires_broken_options_if_non_default( + fn check_remaining_demand_absolute_tolerance_requires_dangerous_options_if_non_default( #[case] value: f64, ) { let flow = Flow::new(value); @@ -434,9 +434,9 @@ mod tests { result, false, value, - "Setting a remaining_demand_absolute_tolerance different from the default value \ - of 1e-12 is potentially dangerous, set \ - please_give_me_broken_results to true if you want to allow this.", + "Setting a remaining_demand_absolute_tolerance different from the default value of \ + 1e-12 is potentially dangerous, set please_give_me_broken_results to true if you want \ + to allow this.", ); }