Fix: Use consistent ordering for commodity prices between runs#601
Merged
Fix: Use consistent ordering for commodity prices between runs#601
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
==========================================
+ Coverage 84.51% 84.54% +0.03%
==========================================
Files 37 37
Lines 3255 3255
Branches 3255 3255
==========================================
+ Hits 2751 2752 +1
+ Misses 318 317 -1
Partials 186 186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR ensures commodity prices are stored in a consistent, sorted order by switching from an IndexMap to a BTreeMap and deriving the necessary ordering traits on key types.
- Derive
Ord/PartialOrdonTimeSliceIDand all ID types to support ordering. - Replace
IndexMapwithBTreeMapforCommodityPrices. - Update imports and macro-derived traits for ID types.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/time_slice.rs | Added Ord/PartialOrd derives to TimeSliceID |
| src/simulation/prices.rs | Swapped IndexMap for BTreeMap in CommodityPrices |
| src/id.rs | Extended define_id_type! to derive Ord/PartialOrd |
Comments suppressed due to low confidence (3)
src/simulation/prices.rs:13
- Consider adding a unit test that inserts a known set of commodity IDs and asserts the iteration order remains consistent across runs when using
BTreeMap.
pub struct CommodityPrices(BTreeMap<(CommodityID, RegionID, TimeSliceID), f64>);
src/simulation/prices.rs:11
- [nitpick] Update this doc comment to note that the map now enforces sorted key ordering by commodity ID for deterministic output.
/// A map relating commodity ID + region + time slice to current price (endogenous)
src/simulation/prices.rs:9
- [nitpick] Switching from
IndexMap(O(1) operations) toBTreeMap(O(log N) operations) may affect performance; consider benchmarking if the map grows large.
use std::collections::{BTreeMap, HashSet};
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
While working on #595 I noticed that commodity prices were in a random order after we removed the contents of
CommodityPrices::add_from_solutionand were therefore all being added (asNaN) inadd_remaining. The issue is that we're using aHashSetto store the IDs of commodities without prices, so, whileCommodityPricesretains insertion order (being anIndexMap), its contents can end up in a random order anyway. We just didn't notice before because we only had one commodity without a price.There are different ways we could fix this, but I've opted to change
CommodityPricesto use aBTreeMaprather than anIndexMap, so the contents are kept ordered according to the commodity ID (i.e. alphabetically). It doesn't particularly matter, but doing it this way means that the commodities are consistently in the same order for different milestone years, which seems like a nice property to have.Type of change
Key checklist
$ cargo test$ cargo docFurther checks