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

Introduce optimization variable #97

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

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jul 2, 2024

This PR introduces the next item of the message data model: Variable. It comes complete with everything the other items already have and only minor changes to Column that are required for this to work, so without further DRY improvements, this PR size is as small as it gets (for the remaining Equation).

Some questions and notes that come up while working on this:

  • We already have a Variable of the iamc category. This requires awkward naming conventions at times. I usually defaulted to OptimizationVariable for this new one, but this leads to a table name of optimization_optimizationvariable (because the docs table is built on the model.__tablename__ -- which of course could be changed by overwriting the __tablename__ locally, and maybe that's preferable) and to OptimizationVariable being importable from ixmp4.core, which I'm not sure we like. Please let me know what you think.
  • How many levels and marginals do Variables (and Equations, for that matter) have? At the moment, I'm assuming that every Key (one row of entries in Variable.data) can have its own level and marginal, making those two just arbitrary columns in Variable.data (though maybe we could limit them to being floats or so).
  • Similarly, I'm wondering: do Variables need to get data added to them manually via an add() function? Or is there data sort of filled in automatically by the gdx code? I don't think I remember lots of variable.create(), variable.add_data() functions from the westeros tutorials.

Working on the transport tutorial, I noticed that I had completely forgotten about adding variables = db.relationship() in the Run DB-table. I have even run migrations for these other PRs, but no error came up. I'm wondering if we even need these relationship() declarations at all.

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

glatterf42 commented Jul 5, 2024

Looking at the transport tutorial, I noticed that it includes a variable like this

Variables
     z       total transportation costs in thousands of dollars ;

I.e, without constraints to indexsets. So I set out to make constrained_to_indexsets optional and in the process, stumbled upon some more questions:

  • For now, variable.columns for an unconstrained variable returns []. (Technically, one might consider levels and marginals to be columns, but we only call things columns that we can bring into optimization.Column form, i.e. constrain to an IndexSet.) We could rework this to return None instead, but at the moment, variable.data returns {} if it's empty since the JsonDict type requires a dictionary, and not None, so [] is consistent with that. So do we want to keep it this way?
  • Should there be a documented workflow for adding Columns to a Variable later on? I guess one could already call the private backend.optimization.variables._add_column(), but it is private, so not ideal to recommend as a default workflow. Once we have a delete() function, we could recommend delete() and create() again. But is there even a need, do people create Variables forgetting that they actually have dimensions and should be constrained to them?
  • I have for now introduced a test like
          # Test that giving column_names, but not constrained_to_indexsets raises
          with pytest.raises(ValueError, match="Received `column_names` to name columns, "
                             "but no `constrained_to_indexsets`"):
              _ = test_mp.backend.optimization.variables.create(
                  run_id=run.id,
                  name="Variable 0",
                  column_names=["Dimension 1"],
              )
    But then I realized that this is a special case of column_names and constrained_to_indexsets not having the same length. So we would catch this error any way, but with a dedicated if check and error message, we could serve people running into this a specific error message, which they might need if they have misunderstood the exact relation and nature of column_names and constrained_to_indexsets. Do we want to keep this or is it enough to remind them that the two parameters need to share the same length (if both are present)?

@glatterf42
Copy link
Member Author

Context for the latest commit: typing.Never can be used to type hint empty lists. Unfortunately, it was only introduced with python 3.11. However, typing.NoReturn was introduced with 3.6.2 and these are equivalent according to the docs.

Copy link
Contributor

@meksor meksor left a comment

Choose a reason for hiding this comment

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

lgtm except for the point about the errors!

@@ -4,6 +4,9 @@
from .optimization.indexset import IndexSet as IndexSet
from .optimization.scalar import Scalar as Scalar
from .optimization.table import Table as Table

# TODO Is this really the name we want to use?
from .optimization.variable import Variable as OptimizationVariable
Copy link
Contributor

Choose a reason for hiding this comment

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

ignoring this for now, more something for daniel i think

ixmp4/data/db/optimization/variable/repository.py Outdated Show resolved Hide resolved
tests/core/test_optimization_variable.py Outdated Show resolved Hide resolved
@meksor
Copy link
Contributor

meksor commented Sep 26, 2024

Hi I investigated the ValueError comment further:
I created a test platform with ixmp4 platforms add test.
Then I started the ixmp4 service in non-manged mode: IXMP4_MANAGED=false ixmp4 server start
Then I executed this test script:

import ixmp4
from ixmp4.conf import settings
from ixmp4.conf.auth import SelfSignedAuth
from ixmp4.conf.manager import PlatformInfo
from ixmp4.data.backend import RestBackend

rb = RestBackend(
    PlatformInfo(name="test", dsn="http://localhost:9000/v1/test/"),
    auth=SelfSignedAuth(settings.secret_hs256),
)

mp = ixmp4.Platform(_backend=rb)


run = mp.runs.create("Model", "Scenario")
ids = mp.backend.optimization.indexsets.create(run_id=run.id, name="Indexset 1")

_ = run.optimization.variables.create(
    "Variable 0",
    constrained_to_indexsets=[ids.name],
    column_names=["Dimension 1", "Dimension 2"],
)

Which results in the following two interactive terminal outputs:
image

Showing us that the ValueError, as expected, is not propagated to the client.

For now you can fix this by making a custom ixmp4 exception, but we should investigate why the test setup cannot reproduce this (i assume because the TestClient class does not prevent server side exceptions being propagated to the client) and how to fix it.

@glatterf42
Copy link
Member Author

glatterf42 commented Sep 30, 2024

Thanks for the instructions and for catching this error! It was fixed in #115 and this branch rebased/reworked to accommodate these changes :)

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