Enforce total capacity limit constraint in investment - #1496
Enforce total capacity limit constraint in investment#1496AdrianDAlessandro wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1496 +/- ##
==========================================
- Coverage 90.29% 90.28% -0.01%
==========================================
Files 60 60
Lines 8645 8700 +55
Branches 8645 8700 +55
==========================================
+ Hits 7806 7855 +49
- Misses 525 531 +6
Partials 314 314 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Yes and yes! Sorry, should have alerted you more to #1495 as it was intended to make your life easier not harder! |
| if best_asset.is_candidate() { | ||
| // Candidate assets: remove capacity from the investment limit, if applicable. | ||
| if let Some(remaining_capacity) = remaining_candidate_capacities.get_mut(&best_asset) { | ||
| if let Some(remaining_capacity) = remaining_addition_limit.get_mut(&best_asset) { | ||
| *remaining_capacity -= best_asset.total_capacity(); | ||
|
|
||
| // If there's not enough capacity remaining to install any more units, remove the | ||
| // asset from the investment options. | ||
| if *remaining_capacity < best_asset.total_capacity() { | ||
| let old_idx = opt_assets | ||
| .iter() | ||
| .position(|asset| *asset == best_asset) | ||
| .unwrap(); | ||
| opt_assets.swap_remove(old_idx); | ||
| remaining_candidate_capacities.remove(&best_asset); | ||
| remaining_addition_limit.remove(&best_asset); | ||
| } | ||
| } | ||
| } else { | ||
| // Commissioned assets: we've appraised a single unit, so remove one unit from the | ||
| // remaining units count for this asset. | ||
| let remaining = remaining_units.get_mut(&best_asset).unwrap(); | ||
| *remaining = remaining.saturating_sub(1); | ||
|
|
||
| // If all units have been selected, remove the asset from the investment options. | ||
| if *remaining == 0 { | ||
| let old_idx = opt_assets | ||
| .iter() | ||
| .position(|asset| *asset == best_asset) | ||
| .unwrap(); | ||
| opt_assets.swap_remove(old_idx); | ||
| remaining_units.remove(&best_asset); | ||
| } |
There was a problem hiding this comment.
I think it's possible this part could fail if the asset has already been removed from the asset options by the total capacity limit (trying to remove the same option twice). You'll need to rework this function a bit to account for this
There was a problem hiding this comment.
I'm not entirely sure what you mean, specifically. But... the more I look at this function, the more I don't understand it and think it doesn't do what it's supposed to do. In the case where the asset drops the remaining capacity to zero, that asset shouldn't be included in the final set of assets used, but it looks like it doesn't behave that way. From what I can tell of this, the asset is added to the list of best assets, but prevented from being added again? Which doesn't really make any sense.
…tal_limit_rough # Conflicts: # src/asset.rs # src/simulation/investment.rs # src/simulation/market.rs
2 participants
Description
This is an attempt at enforcing the total capacity limit constraint in the investment. It still needs some tests, but there are some components I wanted to ask about first.
I had made the total capacity limits mapped by
ProcessID, but was going to ask about the function in which I do that (market.rs::collect_total_limits), because I believe it is looping over duplicate processes and just overwriting theHashMapunnecessarily. However, I have just seen #1495 - which I believe answers that question, but will require me to bring this branch up to date with it and then change my implementation.For now, two questions:
Fixes #1493
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks