Use IndexMap rather than BTreeMap for commodity prices#1040
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1040 +/- ##
=======================================
Coverage 80.71% 80.72%
=======================================
Files 52 52
Lines 6924 6926 +2
Branches 6924 6926 +2
=======================================
+ Hits 5589 5591 +2
Misses 1063 1063
Partials 272 272 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR replaces BTreeMap with IndexMap for storing commodity prices to maintain consistency with the rest of the codebase and provide insertion-order iteration instead of alphabetically sorted iteration. The change affects only the internal data structure of the CommodityPrices struct without altering the API or functionality.
Key Changes
- Changed
CommodityPricesinternal storage fromBTreeMaptoIndexMap - Updated all related type signatures for iterators
- Updated imports to use
indexmap::IndexMapinstead ofstd::collections::BTreeMap
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/simulation/prices.rs |
Core implementation - replaced BTreeMap with IndexMap, updated imports and iterator type signatures |
tests/data/*/commodity_prices.csv |
Test output files - reordered rows to reflect insertion order instead of alphabetical sorting |
tests/data/simple/debug_appraisal_results.csv |
Test output file - minor floating-point precision differences in some values |
The code changes are clean and well-contained. All necessary type signatures have been updated consistently, and the test data changes reflect the expected reordering from alphabetical (BTreeMap) to insertion order (IndexMap).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tsmbland
left a comment
There was a problem hiding this comment.
Seems fine to me. I was never really sure why we were using BTreeMap to begin with.
The only downside I can think of is that, since we add prices in batches according to pricing strategy, changing the pricing strategy of one or more commodities will end up changing the order of prices in the output file. But I don't think anyone will care too much about that
dalonsoa
left a comment
There was a problem hiding this comment.
LGTM, and the explanation makes sense.
Description
We currently store commodity prices in a
BTreeMap. Everywhere else in the code where we need an ordered map, we useIndexMap. While both give consistent ordering when you iterate over them, anIndexMapiterates over items in the same order as they were added (like Pythondicts) and aBTreeMapis sorted by the keys.When I changed commodity prices to use an ordered map in #601, I opted for a
BTreeMapon the grounds that we wouldn't always be adding commodities to the map in the same order and having a consistent ordering in different years seemed like a nice property to have. It was a pretty arbitrary choice though. The downside of ordering by keys is that it also sorts by time slice alphabetically, so e.g.winter.afternooncomes beforewinter.morning. So: commodities possibly ordered slightly better and time slices ordered worse. It doesn't seem worth it.It doesn't particularly matter, but there aren't really any upsides to using a
BTreeMap, so I've changed it to anIndexMapfor consistency with the rest of the code.IndexMaps are faster too, although I imagine the performance improvement will be neglible in our case.This has obviously changed the ordering of the
commodity_prices.csvoutput files, but there shouldn't be any other functional changes.Type of change
Key checklist
$ cargo test$ cargo docFurther checks