Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ impl Asset {
flow.coeff
* prices
.get(&flow.commodity.id, &self.region_id, time_slice)
.unwrap_or_default()
.unwrap_or(MoneyPerFlow(0.0))
Comment on lines 650 to +653
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This continues to treat a missing commodity price as zero via unwrap_or(MoneyPerFlow(0.0)). Given the motivation for removing Default on unit types (to avoid silently inferring missing prices as free), consider changing this call path to propagate absence (e.g., return Option/Result or require callers to provide a fallback explicitly at the call site where it’s truly intended).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is beyond the scope of this PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remember why we were treating missing prices as zero here? Are there cases where we might not have prices yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some commodities genuinely don't have a price (all OTH commodities are unpriced), so it's fair to treat them as free in this case. Other (non-OTH) commodities may have a missing price because there are no producers of that commodity and no potential producers of that commodity in the following year. In this case it's probably NOT a good idea to treat the commodities as free, and this may be responsible for some of the weird prices Adam is describing in #1130

})
.sum()
}
Expand Down
2 changes: 1 addition & 1 deletion src/input/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::rc::Rc;

const ASSETS_FILE_NAME: &str = "assets.csv";

#[derive(Default, Deserialize, PartialEq)]
#[derive(Deserialize, PartialEq)]
struct AssetRaw {
process_id: String,
region_id: String,
Expand Down
14 changes: 11 additions & 3 deletions src/simulation/investment/appraisal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ mod tests {
use crate::fixture::{agent_id, asset, process, region_id};
use crate::process::Process;
use crate::region::RegionID;
use crate::units::{Money, MoneyPerActivity};
use crate::units::{Money, MoneyPerActivity, MoneyPerFlow};
use float_cmp::assert_approx_eq;
use rstest::rstest;
use std::rc::Rc;
Expand Down Expand Up @@ -551,7 +551,11 @@ mod tests {
.map(|(asset, metric)| AppraisalOutput {
asset: AssetRef::from(asset),
capacity: AssetCapacity::Continuous(Capacity(10.0)),
coefficients: ObjectiveCoefficients::default(),
coefficients: ObjectiveCoefficients {
capacity_coefficient: MoneyPerCapacity(0.0),
activity_coefficients: IndexMap::new(),
unmet_demand_coefficient: MoneyPerFlow(0.0),
},
activity: IndexMap::new(),
demand: IndexMap::new(),
unmet_demand: IndexMap::new(),
Expand Down Expand Up @@ -877,7 +881,11 @@ mod tests {
.map(|metric| AppraisalOutput {
asset: AssetRef::from(asset.clone()),
capacity: AssetCapacity::Continuous(Capacity(0.0)),
coefficients: ObjectiveCoefficients::default(),
coefficients: ObjectiveCoefficients {
capacity_coefficient: MoneyPerCapacity(0.0),
activity_coefficients: IndexMap::new(),
unmet_demand_coefficient: MoneyPerFlow(0.0),
},
activity: IndexMap::new(),
demand: IndexMap::new(),
unmet_demand: IndexMap::new(),
Expand Down
1 change: 0 additions & 1 deletion src/simulation/investment/appraisal/coefficients.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ use std::collections::HashMap;
/// the investment appraisal routines. The map contains the per-capacity and per-activity cost
/// coefficients used in the appraisal optimisation, together with the unmet-demand penalty.
#[derive(Clone)]
#[cfg_attr(test, derive(Default))]
pub struct ObjectiveCoefficients {
/// Cost per unit of capacity
pub capacity_coefficient: MoneyPerCapacity,
Expand Down
9 changes: 5 additions & 4 deletions src/simulation/optimisation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -795,10 +795,11 @@ fn calculate_activity_coefficient(
input_prices: Option<&CommodityPrices>,
) -> MoneyPerActivity {
let opex = asset.get_operating_cost(year, time_slice);
let input_cost = input_prices
.map(|prices| asset.get_input_cost_from_prices(prices, time_slice))
.unwrap_or_default();
opex + input_cost
if let Some(prices) = input_prices {
opex + asset.get_input_cost_from_prices(prices, time_slice)
} else {
opex
}
}

/// Calculate the cost coefficient for a capacity variable (for flexible capacity assets only).
Expand Down
7 changes: 5 additions & 2 deletions src/simulation/prices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,10 @@ impl CommodityPrices {
// NB: Time slice fractions will sum to one
let weight = time_slice_info.time_slices[time_slice_id] / Year(1.0);
let key = (commodity_id.clone(), region_id.clone());
*weighted_prices.entry(key).or_default() += *price * weight;
weighted_prices
.entry(key)
.and_modify(|v| *v += *price * weight)
.or_insert_with(|| *price * weight);
}

weighted_prices
Expand Down Expand Up @@ -352,7 +355,7 @@ where
///
/// Note: this should be similar to the "shadow price" strategy, which is also based on marginal
/// costs of the most expensive producer, but may be more successful in cases where there are
// multiple SED/SVD outputs per asset.
/// multiple SED/SVD outputs per asset.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprised this sort of thing doesn't get flagged by any of the checks, to he honest

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... it seems in general there aren't any tools that check doc comments, which is a bit annoying.

///
/// # Arguments
///
Expand Down
2 changes: 0 additions & 2 deletions src/units.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ pub trait UnitType:
+ Sum
+ ApproxEq<Margin = F64Margin>
+ fmt::Display
+ Default
{
/// Create from an f64 value
fn new(value: f64) -> Self;
Expand Down Expand Up @@ -51,7 +50,6 @@ macro_rules! base_unit_struct {
Copy,
PartialEq,
PartialOrd,
Default,
Serialize,
derive_more::Add,
derive_more::Sub,
Expand Down
Loading