Benchmark select_best_assets

[dalonsoa]

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.

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 Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.30%. Comparing base (b36dd9a) to head (cf8a8eb).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1446   +/-   ##
=======================================
  Coverage   90.30%   90.30%           
=======================================
  Files          60       60           
  Lines        8658     8658           
  Branches     8658     8658           
=======================================
  Hits         7819     7819           
  Misses        525      525           
  Partials      314      314           

☔ 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.

[alexdewar]

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.

[dalonsoa]

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.

[tsmbland]

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]

• 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.

[Copilot]

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.

[dalonsoa]

OK, of your concerns @tsmbland , none of them are really actionable if we assume we need to live with the extra complexity, at least in this case. So I would suggest we just merge and go back to it when/if needed.

On your last point, there are a bunch of plots that might be interesting. For example, the index of benchmakrs looks like this:

The file select_best_assets/parallel contains the comparison between using different number of cores. Thar is where the plot in the PR description is, or the following one:

If we want to compare each run with the equivalent previous one, then we need to get into the specific case. For example, for 20 we have the following:

In this case, the last run of the same benchmark was significantly faster that the previous one (who knows why...).

Apparently, there are other plots that can be requested (not sure how) - or linked from the main reports if not directly included. It might be a question of exploring the options and see what plots are really what we want.

[tsmbland]

Ok, let's merge it!

[tsmbland]

Except it needs updating...

[dalonsoa]

Updated

[tsmbland]

Updated

I don't think so... the tests don't pick it up, but pre-commit is failing and the benchmark doesn't work on main

[dalonsoa]

Ehh... what do you mean? True that pre-commit fails, but the tests are all passing, it would seem:

OK, I'm doing it manually

[tsmbland]

Ehh... what do you mean? True that pre-commit fails, but the tests are all passing, it would seem:

OK, I'm doing it manually

The benchmarks aren't covered by the tests, unfortunately. pre-commit is failing because the benchmark is using collect_investment_limits_for_candidates which no longer exists