Create `agent_commodities` table by tsmbland · Pull Request #468 · EnergySystemsModellingLab/MUSE2

tsmbland · 2025-03-31T14:16:25Z

Description

Rather than having a single commodity that an agent can be responsible for, we now allow multiple commodities per agent, and for this to vary year-on-year.

I've gone for a simple approach of storing a Vec<AgentCommodity> for each agent, although it's very possible that this might change once we have a better idea of how this data will be accessed. If I have time I should go through the agent investment documentation and put together some pseudocode. This should give us a much better idea.

Now that this in in place there are some other validation we should do for other tables, e.g. ensuring that search spaces are only applied to commodities that the agent services. In this case it might even make sense to store search spaces within AgentCommodity? Hmm, not sure...

Fixes #453

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

Further checks

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

codecov · 2025-03-31T14:20:57Z

Codecov Report

Attention: Patch coverage is 94.77912% with 13 lines in your changes missing coverage. Please review.

Project coverage is 95.10%. Comparing base (8a7ccd0) to head (80dd6be).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/input/agent/commodity.rs	95.76%	2 Missing and 8 partials ⚠️
src/input/agent.rs	75.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
- Coverage   95.11%   95.10%   -0.01%     
==========================================
  Files          33       34       +1     
  Lines        4752     4947     +195     
  Branches     4752     4947     +195     
==========================================
+ Hits         4520     4705     +185     
- Misses        119      120       +1     
- Partials      113      122       +9

☔ 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

Looks good! I've stuck some comments on there.

I've requested changes because I think the check that the proportions sum to one needs a bigger tolerance.

alexdewar · 2025-04-03T08:08:58Z

+    /// Unique agent id identifying the agent.
+    pub agent_id: String,


Probably don't need this field, though it won't really hurt either

alexdewar · 2025-04-03T08:32:02Z

@@ -0,0 +1,9 @@
+agent_id,commodity_id,year,commodity_portion


After thinking about this, it seems kinda weird to me that we don't also include a region field for the agent commodities stuff (we didn't when it was just a single commodity either so this isn't really about your PR). But how can it make sense for an agent to be responsible for e.g. 50% of the electricity in every region it operates in? Surely it would be different proportions for e.g. the UK and France if the agent operated in both?

Or have I missed something?

This is fine as a simplification for now. But it might be a limitation when it comes to modelling the real world.

Good point and I agree. To be honest I still don't really have an intuitive understanding what it means for an agent to operate in multiple regions. Is it just a shortcut way of saying that we want identical agents in multiple regions, but these are still separate agents for each region (e.g. household agents in UK and France may have approximately similar demand profiles, so we simplify things by using a shared parameter set)? Or is it really a multi-region agent which receives demand from multiple regions and trades commodities between regions (e.g. an oil company)?

I discussed this with Adam last time we spoke and I think there are still open questions here which we/he need to think about. We also discussed scenarios where agents might have no fixed demand share and instead compete against each other to maximize demand share, which is something else that can't really be captured with this table.

Anyway, stuff to think about and bring up when we make plans for the next engagement, but I'd say it's fine to leave it for now

Anyway, stuff to think about and bring up when we make plans for the next engagement, but I'd say it's fine to leave it for now

Agreed 👍

alexdewar · 2025-04-03T08:39:54Z

+            let commodity_id = agent_commodity.commodity.get_id();
+            let portion = agent_commodity.commodity_portion;
+            for region in region_ids {
+                if agent.regions.contains(region) {


Looks like you could just iterate over agent.regions directly here

Hmm no I don't think so because agent.regions is a RegionSelection enum. Unless there's some method I can add to RegionSelection to make it iterable (which would require knowledge of the full set of region_ids). Any ideas?

Oh sorry. You're right, that won't work. Let's leave it as it is.

I have wondered whether we can make RegionSelection as well as SearchSpace simpler by just making them regular sets (for "all" you'd just copy all the IDs into the set). Not a job for today anyway

alexdewar · 2025-04-03T08:46:55Z

+    // We then check the map to ensure values for each key are 1
+    for (key, portion) in summed_portions.iter() {
+        ensure!(
+            (*portion - 1.0).abs() < f64::EPSILON,


I think this might not actually work in practice, because of floating point rounding errors accumulating. Elsewhere I've used float_cmp::approx_eq for this: https://github.com/EnergySystemsModellingLab/MUSE_2.0/blob/8a7ccd00dd53cdbf74030008bd02af4e23aace39/src/input.rs#L200

I had to set the epsilon arg to a surprisingly high value to avoid the check failing with the example model.

alexdewar · 2025-04-03T08:51:00Z

+            )
+        })
+        .map(|(id, _)| Rc::clone(id))
+        .collect();


I don't think you need this collect(). Currently you're iterating over SVD and SED commodities, storing the result, then iterating over that in the loop below. svd_and_sed_commodities could just be an iterator instead. (You might also get away with not cloning the ID if the borrow checker doesn't complain.)

Okay thanks. I've got it working without collect(), but still having to clone the ID. I must admit I still find all this borrowing/referencing stuff very confusing and often end up settling for whatever makes the compiler happy. Definitely need to read that chapter of the rust book again to get a better grasp...

The general rule is you can either have one mutable reference at once or as many immutable references as you want, but I think it would often be pretty hard to keep track of this without the borrow checker!

alexdewar

LGTM!

tsmbland added 2 commits March 31, 2025 15:13

First pass at agent_commodities table

ff1af54

Remove unused imports

ccf5707

tsmbland and others added 4 commits April 1, 2025 10:54

Add validation checks

12b99c4

Add tests

8cd42a7

Add another test

dd8e73a

Merge branch 'main' into agent_commodities

6ae65f4

tsmbland marked this pull request as ready for review April 3, 2025 07:48

tsmbland requested a review from alexdewar April 3, 2025 07:48

alexdewar requested changes Apr 3, 2025

View reviewed changes

tsmbland added 2 commits April 3, 2025 11:46

Apply suggestions from code review

47fd31b

Remove agent_id field from AgentCommodity

80dd6be

tsmbland requested a review from alexdewar April 3, 2025 11:22

alexdewar approved these changes Apr 3, 2025

View reviewed changes

tsmbland merged commit 61070f3 into main Apr 3, 2025
7 checks passed

tsmbland deleted the agent_commodities branch April 3, 2025 12:29

		/// Unique agent id identifying the agent.
		pub agent_id: String,

Conversation

tsmbland commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov Bot commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tsmbland commented Mar 31, 2025 •

edited

Loading

codecov Bot commented Mar 31, 2025 •

edited

Loading