Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include the linopy version of the transport tutorial #101

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jul 10, 2024

Note

This PR looks huge, but it actually isn't. Please focus on py_transport.ipynb and linopy_model.py in transport/tutorial.

The reason this PR looks so huge is because all my others PRs are still waiting for reviews but they are actually needed to enable the transport tutorial. Thus, I have git cherry-picked the required commits from each one of them, increasing the apparent amount of changes in here. However, the actually important changes are only about 200 lines long, so please bare with me.

First of, this version is using linopy because that was more readily available than learning how to use gamsapi (which I will do next, regardless) and because it might help to then abstract some ixmp4-solver package or some such so that it doesn't lean too much on the gams formulation. Linopy support a wide range of solvers, of which I have employed the highs solver. I have installed the full highs software locally and added the highspy package as a dependency. I'm not sure if just the python package is enough; please install the full software in that case (all check should still pass because ...

  • ... we don't have any test for the tutorial yet).

Linopy also supports gurobi, which unfortunately does not work with numpy 2.0 yet. Hence linopy's pin of numpy 2.0. Since we do want to support recent numpy versions, though, I have created a branch on my linopy fork that removes the numpy pin and added this branch as a dependency here. Please note that removing this pin does break linopy's test suite locally, namely the test relying on gurobipy. Please also note that this branch may go out of date since linopy is actively developed and I don't know how frequent I can update my fork.

This PR contains two changes to the data validation functions for Equations and Variables because we actually only need to validate data if there is data to validate. The corresponding commit should be cherry-picked to the other branches so that they can be merged and this PR can be rebased, or it can be included here.

Similarly, this PR contains a commit introducing all missing DB migrations, which is a lot. These might still change due to the pending reviews on other PRs, so this commit is marked as TEMPORARY.

Finally, the actual tutorial code is included in the files mentioned in the beginning.
GAMS would need a model file, if I understand correctly, to record all parameters, variables, and equations properly. Linopy would not need that, but I figured I could try to abstract data from functionality and so have put the functionality almost completely into linopy_model.py. Doing so means that the tutorial itself only needs a few lines of code to work with linopy, which is probably not enough to warrant a new ixmp4-solver package.
But please share your thoughts on this :)

@glatterf42 glatterf42 added the enhancement New feature or request label Jul 10, 2024
@glatterf42 glatterf42 self-assigned this Jul 10, 2024
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Thanks @glatterf42, this is coming together nicely! I made a few suggestions for streamlining, hope that makes sense...

tutorial/transport/py_transport.ipynb Outdated Show resolved Hide resolved
tutorial/transport/py_transport.ipynb Outdated Show resolved Hide resolved
tutorial/transport/py_transport.ipynb Outdated Show resolved Hide resolved
"scen.solve(model='dantzig')"
"from tutorial.transport.linopy_model import create_dantzig_model, store_dantzig_solution\n",
"\n",
"m = create_dantzig_model(i=i, j=j, a=a, b=b, d=d, f=f)\n",
Copy link
Member

Choose a reason for hiding this comment

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

The Dantzig model code knows which sets and parameters are needed so this can all be read from the run directly...

Suggested change
"m = create_dantzig_model(i=i, j=j, a=a, b=b, d=d, f=f)\n",
"m = create_dantzig_model(run)\n",

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that and I'm not sure I agree. Passing only a Run means we have to document the requirements for this Run: it needs to have these IndexSets, Parameters, etc. The way it is now, we only explicitly pass in the data we need, or maybe even still too much. In practice, it might suffice to just pass in i.elements and i.name etc.

Copy link
Member

Choose a reason for hiding this comment

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

But think about the signature for creating a message_ix model, which has dozens of sets and parameters. This will become extremely painful.

In practice, the "model" knows which sets and parameters are required, so if a Run is passed that doesn't have a required item, the model (i.e., the model-creation-function) will just raise an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically, the signature for something like create_message_model() could be different from create_dantzig_model(), but fine, I'll just pass a run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please note, though, that passing a run requires additional DB calls to get() the indexsets etc, which likely slows down runtime. I doubt that it will notably affect the transport tutorial, though.

tutorial/transport/py_transport.ipynb Outdated Show resolved Hide resolved
tutorial/transport/linopy_model.py Outdated Show resolved Hide resolved
tutorial/transport/linopy_model.py Outdated Show resolved Hide resolved
tutorial/transport/linopy_model.py Outdated Show resolved Hide resolved
tutorial/transport/linopy_model.py Outdated Show resolved Hide resolved
tutorial/transport/linopy_model.py Outdated Show resolved Hide resolved
@glatterf42
Copy link
Member Author

glatterf42 commented Jul 11, 2024

Good news first: we now have tests for the tutorial :)
Bad news:

  • I'm not entirely sure why one of them isn't working, commented that one out for now (not too familiar with xarray yet, I'd prefer to use pandas). Edit: tests are working now (except for linopy still messing with mypy :)
  • I had to duplicate the model file (more or less) because I didn't know where to put it in relation to existing files. This might be fine if we actually migrate all tutorial/solver stuff into their own package, otherwise, @meksor, please let me know where to put the files.

Still wanted to push these changes before going on leave.

glatterf42 and others added 20 commits October 3, 2024 11:25
* Covers:
* run__id, data, name, uniqueness of name together with run__id
* Adapts tests since default order of columns changes
* Make Column generic enough for multiple parents
* Introduce optimization.Parameter
* Add tests for add_data
* Enable remaining parameter tests (#86)
* Enable remaining parameter tests
* Include optimization parameter api layer (#89)
* Bump several dependency versions
* Let api/column handle both tables and parameters
* Make api-layer tests pass
* Include optimization parameter core layer (#90)
* Enable parameter core layer and test it
* Fix things after rebase
* Ensure all intended changes survive the rebase
* Adapt data validation function for parameters
* Allow tests to pass again
@glatterf42 glatterf42 force-pushed the include/transport-tutorial-linopy branch from c86f7be to 4e02ecb Compare October 3, 2024 10:29
@glatterf42
Copy link
Member Author

@danielhuppmann I rebased this branch and included several fixes so the tests should be fine again. However, if I remember correctly, you once said that we might want to include the optimization.solve functionality only as a subpackage or similar. Do you have a preferred way of integrating the functionality now that it could be included (after some clean up)?

Please note (mostly to self): I forgot to include the DB migrations with the Parameter, Variable, and Equation PRs. They are included here, but if we end up not merging this branch, I should cherry-pick them or run them again on main.

@danielhuppmann
Copy link
Member

@danielhuppmann we might want to include the optimization.solve functionality only as a subpackage or similar. Do you have a preferred way of integrating the functionality now that it could be included (after some clean up)?

If I understand this correctly, the solve method is part of linopy anyway.

Looking at the PR quickly, it seems like the current approach is - makes sense to me.

m = linopy.Model()
# transfer sets and parameters to `m`
m.solve()
# extract solution and save as Variables

And for more elaborate model-frameworks like MESSAGEix, this can be part of the message_ix package, like

run = message_ix.Scenario()  # this implicitly creates the sets, parameters, variables
# populate the sets and parameters with data
run.solve()  # which transfers sets and parameters, calls `solve()`, and extracts the solution and saves as Variables
# get variables from `run` to analyze the results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants