Group child assets in commodity_flows.csv

[mjademitchell]

Description

Added an optional group_id column and made asset_id optional to support both parent and regular assets.
Added skipping child assets when writing the file and write the parent's flows instead

Fixes #1242

Type of change

• Bug fix (non-breaking change to fix an issue)
• New feature (non-breaking change to add functionality)
• Refactoring (non-breaking, non-functional change to improve maintainability)
• Optimization (non-breaking change to speed up the code)
• Breaking change (whatever its nature)
• Documentation (improve or add documentation)

Key checklist

• All tests pass: $ cargo test
• The documentation builds and looks OK: $ cargo doc
• Update release notes for the latest release if this PR adds a new feature or fixes a bug
  present in the previous release

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[mjademitchell]

When I run the cargo run -- run examples/simple --overwrite it works but cargo test it fails so in my next commit ill work on the regression tests

[alexdewar]

This needs one more little tweak to work. We're currently stripping out the entries for parent assets from the FlowMap in create_flow_map:

MUSE2/src/simulation/optimisation.rs

Line 218 in 78fde8d

flows.retain(|(asset, _, _), _| !asset.is_parent());

So you need to remove that line, otherwise we won't have children or parents in commodity_flows.csv. If you look at the results for simple_divisible (cargo run example run --patch simple_divisible) then you should see entries for RSHEAT with group IDs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.46%. Comparing base (0af4399) to head (58738dd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1286   +/-   ##
=======================================
  Coverage   89.46%   89.46%           
=======================================
  Files          57       57           
  Lines        8361     8364    +3     
  Branches     8361     8364    +3     
=======================================
+ Hits         7480     7483    +3     
  Misses        581      581           
  Partials      300      300           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alexdewar]

I had a look at the output files just to check that they were giving us what was expected and found that the flow values for parent assets are the same as they were for child assets before, when they should be [num children] times as big. So there's something I forgot. Oops.

Looking at create_flow_map, it looks like we divide the flows for parent assets by the number of children, which was fine when we were chucking those entries away, but now that we need them, the values are wrong.

I was able to fix it like this:

diff --git a/src/simulation/optimisation.rs b/src/simulation/optimisation.rs
index 81afd181..66f3f448 100644
--- a/src/simulation/optimisation.rs
+++ b/src/simulation/optimisation.rs
@@ -191,10 +191,9 @@ fn create_flow_map<'a>(
     // multiply this value by the flow coeffs to get commodity flows.
     let mut flows = FlowMap::new();
     for (asset, time_slice, activity) in activity {
-        let n_units = Dimensionless(asset.num_children().unwrap_or(1) as f64);
         for flow in asset.iter_flows() {
             let flow_key = (asset.clone(), flow.commodity.id.clone(), time_slice.clone());
-            let flow_value = activity * flow.coeff / n_units;
+            let flow_value = activity * flow.coeff;
             flows.insert(flow_key, flow_value);
         }
     }
@@ -202,12 +201,13 @@ fn create_flow_map<'a>(
     // Copy flows for each child asset
     for asset in existing_assets {
         if let Some(parent) = asset.parent() {
-            for commodity_id in asset.iter_flows().map(|flow| &flow.commodity.id) {
-                for time_slice in time_slice_info.iter_ids() {
+            let n_units = Dimensionless(asset.num_children().unwrap_or(1) as f64);
+            for time_slice in time_slice_info.iter_ids() {
+                for commodity_id in asset.iter_flows().map(|flow| &flow.commodity.id) {
                     let flow = flows[&(parent.clone(), commodity_id.clone(), time_slice.clone())];
                     flows.insert(
                         (asset.clone(), commodity_id.clone(), time_slice.clone()),
-                        flow,
+                        flow / n_units,
                     );
                 }
             }

(I also reordered a couple of loops so the flows for parent assets are ordered the same way as for non-divisible assets.)

You'll also need to regenerate test data after this change.

Sorry! My bad.

There's also a comment we can now remove.

[mjademitchell]

@alexdewar I have tried to fix this many time sbut cargo test on github CI keeps failing but in my local it works i ran the just regenerate_data even the just regenerate_data --patch simple_divsible and then i do cargo test it works perfectly fine everything pass but when i push to the branch its failing have i missed something

[alexdewar]

@mjademitchell That's strange! You definitely don't have uncommitted changes in your local copy? I can see you updated the commodity_flows.csv file in your last commit, but for some reason it's also failing on my local machine (both the commodity_flows.csv and assets.csv files don't match).

Could you try regenerating the test data for simple_divisible again, committing and pushing?

[mjademitchell]

@mjademitchell That's strange! You definitely don't have uncommitted changes in your local copy? I can see you updated the commodity_flows.csv file in your last commit, but for some reason it's also failing on my local machine (both the commodity_flows.csv and assets.csv files don't match).

Could you try regenerating the test data for simple_divisible again, committing and pushing?

For some reason its still not working!