Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR limits the search space in the model to only include technologies that produce at least one demanded commodity, thereby reducing the size of the underlying mathematical problem.
- Added filtering logic in initialize_from_technologies in src/muse/filters.py.
- Updated tests in tests/test_filters.py to verify correct behavior when no demanded commodities are produced.
- Modified tests in tests/test_agents.py to account for region-specific selection when invoking retro_agent.next.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_filters.py | Added tests to ensure search_space correctly reflects commodity demand. |
| tests/test_agents.py | Updated retro_agent.next call to perform selection by region. |
| src/muse/filters.py | Introduced filtering logic to combine search_space with demanded commodity mask. |
Comments suppressed due to low confidence (2)
src/muse/filters.py:456
- [nitpick] Ensure that the boolean mask 'produces_demanded_commodity' correctly broadcasts to all dimensions of 'search_space'. Including an inline comment or an assert to document the expected dimensional consistency could help prevent subtle filtering issues.
return search_space & produces_demanded_commodity
tests/test_agents.py:147
- [nitpick] Consider adding a comment to clarify the rationale behind hardcoding the region selection to index 0 or, if applicable, extend the test data to cover scenarios with multiple regions.
retro_agent.next(technologies.sel(year=investment_year).isel(region=0), agent_market.isel(region=0), demand_share,)
dalonsoa
approved these changes
Mar 27, 2025
Collaborator
dalonsoa
left a comment
There was a problem hiding this comment.
I think it makes perfect sense the way you implemented it. It is surprising this was not already in place - why investing in a technology that does not produce any demanded commodity?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adding some code to
initialize_from_technologies, which initializes the search space, to only consider technologies that produce at least one demanded commodity. Ultimately, if there are any technologies in the sector that do not produce any demanded commodities, removing them from the search space will shrink the size of the linear problem at no cost.This will be particularly important once I've done #684 and users can split commodity demands across multiple subsectors. This should then be a viable solution for fixing the memory problems in #389 (watch this space...).
I considered making this a separate filter, but I want it to be included in all models, for all agents, regardless of what filters the user has put in their agents file, so this seemed like the most appropriate place to put it.
Added a test to show that it works.
There was one stubbornly failing test (
test_run_retro_agent), which I've managed to keep happy in the end. Not too fussed about it though as it's to do with retrofit agents which are on their way out anyway