Skip to content

Benchmark select_best_assets - #1446

Open
dalonsoa wants to merge 8 commits into
mainfrom
1444_assets_bench
Open

Benchmark select_best_assets#1446
dalonsoa wants to merge 8 commits into
mainfrom
1444_assets_bench

Conversation

@dalonsoa

Copy link
Copy Markdown
Collaborator

Description

Very much Copilot's work, with me supervising, to be honest. As pointed out, the input to select_best_assets is very complicated, so I asked Copilot to create such in input starting from the two_outputs example. Then, I elaborated making a sweep on the number of competing technologies. To avoid code duplication, some functions in MUSE2 were made public, but otherwise left unchanged.

To run it use:

cargo bench --bench assets

Or for a quick run:

cargo bench --bench assets -- --quick

One of the outputs is the following plot, which as expected shows a linear dependency with the number of competting assets. When parallelize, we would - hopefully - see a different behaviour.

Screenshot from 2026-07-28 12-51-44

Fixes #1444

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

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.78%. Comparing base (8d7b817) to head (a77f0f3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1446      +/-   ##
==========================================
+ Coverage   89.71%   89.78%   +0.07%     
==========================================
  Files          60       60              
  Lines        8439     8439              
  Branches     8439     8439              
==========================================
+ Hits         7571     7577       +6     
+ Misses        551      545       -6     
  Partials      317      317              

☔ View full report in Codecov by Harness.
📢 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.

@dalonsoa dalonsoa changed the title 1444 assets bench Benchmark select_best_assets Jul 28, 2026
@dalonsoa dalonsoa linked an issue Jul 28, 2026 that may be closed by this pull request

@alexdewar alexdewar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cool! Nothing to add really, except that it will be interesting to see how this changes with parallelisation 😄.

I suppose this would potentially make it a little harder to change the interface for select_best_assets as the benchmark will also need updating in that case, but I guess this is the kind of thing that LLMs are good at fixing anyway.

Also, might we want to verify that benchmarks at least compile and maybe run to completion as a CI workflow? Given that we won't be looking at the numbers in this context, we could possibly run it with a reduced number of iterations, if that's an option.

Comment thread benches/assets.rs Outdated
@dalonsoa

Copy link
Copy Markdown
Collaborator Author

Also, might we want to verify that benchmarks at least compile and maybe run to completion as a CI workflow? Given that we won't be looking at the numbers in this context, we could possibly run it with a reduced number of iterations, if that's an option.

Let's do this as part of #1453 . There's a --quick option precisely for this.

Co-authored-by: Alex Dewar <[email protected]>
@tsmbland

tsmbland commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Looks good! On board with this in principal, just a few comments:

  • The code for generating the inputs looks ok (on a quick glance), albeit long and convoluted. I'm a bit worried about this kind of thing becoming a burden if we ever want to change the code (the same way that the tests in MUSE1, which are full of similarly complex data preparation, have been a total liability). Perhaps I should be less worried about this in the age of AI...
  • In general, might it be useful to share code between test fixtures and benchmarks? Not sure the best approach, but seems silly if there are already test fixtures that could be useful not to use them in benchmarks, and similarly if we go to all the effort to prepare data for benchmarks, this might also allow us to run more sophisticated unit tests.
  • If I understand it runs all the 1's then all the 2's etc? I wonder if it might be a bit more robust to mix this up? For example, if I happened to do something else on my laptop while it's running all the 14's, could we get a weird dip at 14?
  • Not sure how I feel about making things public just for the purposes of benchmarking, as generally I think it's quite a useful safeguard to limit what's public.
  • The line chart you showed above is particularly useful, but doesn't show a comparison against the previous benchmark, which is probably where we'd want it the most.

@dalonsoa

Copy link
Copy Markdown
Collaborator Author
  • The code for generating the inputs looks ok (on a quick glance), albeit long and convoluted. I'm a bit worried about this kind of thing becoming a burden if we ever want to change the code (the same way that the tests in MUSE1, which are full of similarly complex data preparation, have been a total liability). Perhaps I should be less worried about this in the age of AI...

I was reluctant to this complex setup, but for functions with complex inputs, there not that many options.

  • In general, might it be useful to share code between test fixtures and benchmarks? Not sure the best approach, but seems silly if there are already test fixtures that could be useful not to use them in benchmarks, and similarly if we go to all the effort to prepare data for benchmarks, this might also allow us to run more sophisticated unit tests.

I agree. In Python it would be a piece of cake, but in rust things seem to be quite separated and I'm not sure how easy that would be. We could explore the options, though.

  • If I understand it runs all the 1's then all the 2's etc? I wonder if it might be a bit more robust to mix this up? For example, if I happened to do something else on my laptop while it's running all the 14's, could we get a weird dip at 14?

Indeed, that might happen. Just don't do anything else while doing the benchmarking 😅 . Jokes aside, we could explore the options, but it might be simpler to just re-run the benchmark if you spot a weird behaviour.

  • Not sure how I feel about making things public just for the purposes of benchmarking, as generally I think it's quite a useful safeguard to limit what's public.

Yeah, probably not great making things public unless needed.

  • The line chart you showed above is particularly useful, but doesn't show a comparison against the previous benchmark, which is probably where we'd want it the most.

I haven't explored the options much. There were other plots in the report, and then you could access the individual reports for 1s, 2s, etc. maybe there is a more useful plot. I cannot check today, but can have a look tomorrow.

Base automatically changed from benchmark_examples to main August 3, 2026 08:49
Copilot AI lite review requested due to automatic review settings August 4, 2026 09:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a Criterion benchmark to measure how select_best_assets scales as the number of competing candidate technologies increases, using the two_outputs example model as a realistic input basis. To enable the benchmark to reuse existing internal setup logic without duplicating it, several helper functions were made public and Process was made Clone.

Changes:

  • Add benches/assets.rs Criterion benchmark sweeping the number of competing technologies.
  • Expose several simulation helper functions (get_*, flatten_preset_demands_for_year, candidate_assets_for_next_year) for benchmark reuse.
  • Derive Clone for Process and register the new bench target in Cargo.toml.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/simulation/market.rs Makes market helper functions public for benchmark setup.
src/simulation/investment.rs Makes demand-flattening helper public for benchmark setup.
src/simulation.rs Makes candidate_assets_for_next_year public for benchmark setup.
src/process.rs Derives Clone for Process to support synthetic process generation in the benchmark.
Cargo.toml Registers the new assets benchmark target.
benches/assets.rs Adds the select_best_assets scaling benchmark implementation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread benches/assets.rs
Comment on lines +161 to +165
let templates: Vec<Rc<Process>> = agent
.iter_search_space(region_id, &commodity.id, YEAR)
.cloned()
.collect();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Benchmark select_best_assets

4 participants