Skip to content

Tidy scipy adapter code#717

Merged
tsmbland merged 37 commits intomainfrom
lp_adapter
Jun 10, 2025
Merged

Tidy scipy adapter code#717
tsmbland merged 37 commits intomainfrom
lp_adapter

Conversation

@tsmbland
Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland commented Jun 2, 2025

Tidying up the scipy adapter code, based on this suggestion:

I always thought this was a very odd design pattern. Why not simply storing the templates in attributes of ScipyAdapter and making the relevant methods like back_to_muse not static? It will be way simpler to follow.
All in all, and considering the size of this file, I think it will make sense to extract this as in its own module and make most of these static methods just plain functions in there. They way they are used here feels like the wrong way of using static methods, specially when you are calling them within the class itself.
Originally posted by @dalonsoa in #685 (comment)

The scipy adapter code now lives in a new file (lp_adapter.py), separate from the constraints code, and uses a simpler design pattern, as suggested.

I've also reworked the tests a bit, mostly to tidy up the fixtures and put the tests in a more logical order. I think this is an improvement. Despite splitting the lp adapter tests into a new file, I haven't split the tests into a new file. The reason is that the constraints and lp adapter tests share pretty much the same fixtures, so to avoid duplicating fixture definitions it made more sense to keep the tests in the same file. Maybe there's a better solution though - happy for suggestions

@tsmbland tsmbland requested a review from Copilot June 2, 2025 22:11
Copy link
Copy Markdown

Copilot AI left a comment

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 extracts the LP adapter logic into a new lp_adapter.py module and updates references in investments.py to use the new adapter.

  • Introduce src/muse/lp_adapter.py containing utilities to convert MUSE xarray structures for SciPy LP.
  • Update src/muse/investments.py to import and call ScipyAdapter.from_muse_data from the new module.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/muse/lp_adapter.py New module with conversion functions and adapter
src/muse/investments.py Updated import path and factory call for adapter
Comments suppressed due to low confidence (1)

src/muse/lp_adapter.py:54

  • [nitpick] The loop variable u is ambiguous. Consider renaming it to something more descriptive like var_name to improve readability.
result = data[[u for u in data.data_vars if str(u).startswith(name)]]

Comment thread src/muse/lp_adapter.py
@EnergySystemsModellingLab EnergySystemsModellingLab deleted a comment from Copilot AI Jun 3, 2025
from __future__ import annotations

Restore some deleted test behaviour

Split tests

Revert tests
@tsmbland tsmbland force-pushed the lp_adapter branch 2 times, most recently from 0cdec3b to 1cfc24f Compare June 5, 2025 17:52
@tsmbland tsmbland changed the base branch from main to tidy_tests June 5, 2025 17:52
@tsmbland tsmbland changed the title Tidy lp adapter, move to new module Tidy scipy adapter code Jun 6, 2025
Base automatically changed from tidy_tests to main June 10, 2025 11:06
@tsmbland tsmbland marked this pull request as ready for review June 10, 2025 13:36
@tsmbland tsmbland requested a review from dalonsoa June 10, 2025 13:37
Copy link
Copy Markdown
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

I have not gone into the detail of the code, but this is way, way more readable and, specially, the code structure makes sense, facilitating understanding the workflow of the application. I would not worry too much about the structure of the tests.

@tsmbland tsmbland merged commit 0788d07 into main Jun 10, 2025
14 checks passed
@tsmbland tsmbland deleted the lp_adapter branch June 10, 2025 14:01
@github-project-automation github-project-automation Bot moved this to ✅ Done in MUSE Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants