Refactor: Move future and decommissioned assets out of AssetPool#1114
Refactor: Move future and decommissioned assets out of AssetPool#1114
AssetPool#1114Conversation
This reverts commit 637b465.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1114 +/- ##
==========================================
- Coverage 87.52% 87.48% -0.04%
==========================================
Files 55 55
Lines 7429 7415 -14
Branches 7429 7415 -14
==========================================
- Hits 6502 6487 -15
- Misses 630 631 +1
Partials 297 297 ☔ 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 refactors the asset management code by separating concerns: instead of storing active, future, and decommissioned assets together in AssetPool, the code now stores user-defined assets in the Model struct and maintains separate collections for active and decommissioned assets during simulation. This improves the separation of concerns and eliminates lifetime issues that arose from needing multiple mutable references to the same structure.
Changes:
- Refactored
AssetPoolto only contain active assets, removing thefutureanddecommissionedfields - Moved user-defined assets to the
Modelstruct as a newuser_assetsfield - Changed
load_modelto return justModel(which now includes user_assets) instead of a tuple - Updated the simulation
runfunction to manage user assets and decommissioned assets separately
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simulation.rs | Updated to clone user_assets from Model, manage active and decommissioned pools separately, and chain them when writing to file |
| src/model.rs | Added user_assets field to store user-defined assets as part of the model input data |
| src/input/asset.rs | Renamed read_assets to read_user_assets and changed return type from Vec<Asset> to Vec<AssetRef> |
| src/input.rs | Modified load_model to return just Model (containing user_assets) instead of a tuple |
| src/fixture.rs | Updated test fixtures to work with the new API (empty AssetPool creation, explicit commissioning) |
| src/cli.rs | Updated to handle the new load_model return type and run function signature |
| src/asset.rs | Refactored AssetPool structure, made commission_new and decommission_old/mothballed public, changed them to work with external user_assets and return iterators of decommissioned assets |
| src/output.rs | Updated test method calls from iter_active() to iter() to match the new API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/asset.rs
Outdated
| @@ -1425,10 +1375,10 @@ impl AssetPool { | |||
| } | |||
|
|
|||
| // New assets may not have been sorted, but active needs to be sorted by ID | |||
There was a problem hiding this comment.
The comment says "active needs to be sorted by ID" but the field is now called "assets". Update to "assets needs to be sorted by ID" or "the asset pool needs to be sorted by ID".
| // New assets may not have been sorted, but active needs to be sorted by ID | |
| // New assets may not have been sorted, but assets need to be sorted by ID |
src/asset.rs
Outdated
| pub fn commission_new(&mut self, year: u32, user_assets: &mut Vec<AssetRef>) { | ||
| let to_commission = user_assets.extract_if(.., |asset| asset.commission_year <= year); | ||
|
|
||
| // Move assets from future to active |
There was a problem hiding this comment.
The comment "Move assets from future to active" is outdated. With the refactored design, user assets are no longer called "future" assets, and they're now stored externally. Consider updating this comment to something like "Commission assets from user_assets that are due this year" to better reflect the new architecture.
| // Move assets from future to active | |
| // Commission assets from user_assets that are due on or before this year into the active pool |
src/asset.rs
Outdated
| @@ -1348,36 +1329,25 @@ impl AssetPool { | |||
| pub fn get(&self, id: AssetID) -> Option<&AssetRef> { | |||
| // The assets in `active` are in order of ID | |||
There was a problem hiding this comment.
The comment references "active" but the field is now called "assets". Update the comment to say "The assets in assets are in order of ID" or simply "Assets are stored in order of ID".
| // The assets in `active` are in order of ID | |
| // The assets in `assets` are in order of ID |
|
Actually @Aurashk I've realised this needs more work, so maybe hold off on review for now |
1 similar comment
|
Actually @Aurashk I've realised this needs more work, so maybe hold off on review for now |
Returning an iterator has a subtle gotcha: because iterators are evaluated lazily, if the caller does not comsume the whole thing, only some of the assets that should be decommissioned will be.
|
@aurash I've tweaked this now. My original implementation returned decommissioned assets as iterators, but this has a subtle Anyway, I think this is ready for review now. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Write assets to file | ||
| writer.write_assets(assets.iter_all())?; | ||
| writer.write_assets(asset_pool.iter())?; |
There was a problem hiding this comment.
In the base year, only active assets are written to the output file, but decommissioned assets should also be included for consistency. If any user-defined assets have already reached their max_decommission_year by the base year, they would be skipped during commissioning (with a warning) but never written to the output file. This creates an inconsistency with subsequent years (line 133) where both decommissioned and active assets are written. Consider changing this line to: writer.write_assets(decommissioned.iter().chain(asset_pool.iter()))?; to match the pattern used in subsequent years.
There was a problem hiding this comment.
This isn't right... assets that are "decommissioned" before the time horizon are simply discarded, so they never end up in the decommissioned Vec. So there are no decommissioned assets to be written to file here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut writer = DataWriter::create(output_path, &model.model_path, debug_model)?; | ||
| let mut user_assets = model.user_assets.clone(); | ||
| let mut asset_pool = AssetPool::new(); // active assets | ||
| let mut decommissioned = Vec::new(); |
There was a problem hiding this comment.
The decommissioned Vec is never cleared between milestone years, causing decommissioned assets to accumulate across all years. When assets are written at line 133, all previously decommissioned assets from earlier years will be written again, creating duplicate entries in the output. The Vec should be cleared after writing assets to file (after line 133) to ensure each year only writes the assets that were decommissioned in that specific year.
There was a problem hiding this comment.
Yes, but that's by design! We write all decommissioned assets to file.
Unsure why `cargo fmt` didn't fix this... Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
alexdewar
force-pushed
the
separate-out-asset-pool
branch
from
9a1deeb to
ae12d93
Compare
1 participant
Description
I've been trying to refactor the asset code for #1100, but I realised that we don't have a particularly good separation of concerns for
AssetPool.Currently, it contains all active, as well as all future and all decommissioned assets. Bundling in the latter two with the active assets doesn't make sense, I think, and causes irritating lifetime issues, e.g. when trying to extract future assets and append them to the active pool (you can't have two mutable refs to self). We can work around the lifetime issues, but I think it's a sign that these data structures don't really belong together.
So I've refactored it so that there are now separate
Vecs for future (user-defined) assets and decommissioned assets. I think this makes for a nicer API overall, actually: for example, we can now just return decommissioned assets as an iterator rather than making assumptions about what should be done with them.I also moved the user-defined assets to be stored in
Modelalong with the other input data, which I think make sense, so we now have to copy them at the start of the simulation to get a mutableVec(though we only do this once).Let me know what you think 😄.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks