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 optimization scalar #28

Merged
merged 34 commits into from
Jan 25, 2024
Merged

Include optimization scalar #28

merged 34 commits into from
Jan 25, 2024

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Nov 8, 2023

Summary

This PR expands the data model required for message_ix by adding limited support for Scalars. One can now create, get, list, tabulate, and manage docs for scalars with the following syntax:

Implementation Details

# In the core layer:
test_mp = Platform(_backend=RestTestBackend(test_sqlite_mp.backend))
run = test_mp.runs.create("Model", "Scenario")
unit = test_mp.units.create("Test Unit")
# To create a new scalar:
# NOTE: the combination of (Run.id, Scalar.name) is checked for uniqueness
        scalar_1 = run.optimization.scalars.create(
            "Scalar 1", value=10, unit="Test Unit"
        )

# To list existing scalars:
scalars = run.optimization.scalars.list()

# To get a single scalar:
scalar = run.optimization.scalars.list(name="Scalar 1")
# or:
scalar = run.optimization.scalars.get("Scalar 1")

# To tabulate a scalar by specifying a name:
scalar_1 = run.optimization.scalars.tabulate(name="Scalar 1")

# To update a scalar:
        scalar = run.optimization.scalars.create(
            "Scalar", value=10, unit=unit.name
        )
        assert scalar.value == 10
        assert scalar.unit.id == unit.id

        scalar.value = 30
        scalar.unit = "Test Unit"

        result = run.optimization.scalars.get("Scalar")

        assert scalar.id == result.id
        assert scalar.name == result.name
        assert scalar.value == result.value == 30
        assert scalar.unit.id == result.unit.id == 1

# See tests/data/test_optimization_scalar.py to see the syntax for the data layer.

Further Changes

While working on this PR, I added/updated a few other minor things:

  • Added tests for the data layer functionality of IndexSets
  • Updated/expanded IndexSet tests
  • Added a data-layer get_by_id function for Units

TO-DOs

  • Add migrations once the DB model is accepted

@glatterf42 glatterf42 added the enhancement New feature or request label Nov 8, 2023
@glatterf42 glatterf42 self-assigned this Nov 8, 2023
@danielhuppmann

This comment was marked as resolved.

@glatterf42

This comment was marked as resolved.

@glatterf42
Copy link
Member Author

Please note that there is support for upsert operations in sqlalchemy for sqlite and postgresql. I opted for separate create() and update() functionality because our current create() functions employ the session.add statement, which seems to be different from the INSERT statement that is required for upserts. We would probably also have to manage the different dialect-specific insert() functions in order to employ their insert().on_conflict_do_update() functionality, since this doesn't exist for all backends. Thus, keeping create() and update() separate seemed easier and more aligned with current infrastructure. If more update functionality is desired, it might however be beneficial to look into the existing upsert functions and maybe add an Upserter mixin.

@danielhuppmann

This comment was marked as resolved.

@glatterf42

This comment was marked as resolved.

@danielhuppmann

This comment was marked as resolved.

@glatterf42 glatterf42 force-pushed the include/optimization-scalar branch from 85fc68c to 7055420 Compare November 14, 2023 14:55
@glatterf42 glatterf42 requested a review from meksor November 14, 2023 15:08
ixmp4/core/optimization/scalar.py Outdated Show resolved Hide resolved
ixmp4/data/db/optimization/scalar/model.py Outdated Show resolved Hide resolved
ixmp4/data/db/run/model.py Outdated Show resolved Hide resolved
ixmp4/db/__init__.py Outdated Show resolved Hide resolved
ixmp4/server/rest/optimization/scalar.py Show resolved Hide resolved
tests/core/test_indexset.py Show resolved Hide resolved
indexset_2 = run.optimization.IndexSet("Indexset 2")
exp = df_from_list(indexsets=[indexset_1, indexset_2])
res = run.optimization.indexsets.tabulate()
# utils.assert_unordered_equality doesn't like lists, so make sure the order in
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "doesnt like lists" mean? assert_unordered_equality is meant only for DFs afaik...

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, that's why it didn't work when I tried to pass in a list of DFs. That's why I added this auxiliary df_from_list() function. We could rephrase this hint as "... only accepts pd.DataFrames,". Or we remove this comment entirely, but both the order of indexset_1, indexset_2 as arguments for df_from_list() as well as the order of indexset.name, indexset.elements, ... in the function's return-value-DF are important for the test to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'm kind of confused by that. Nevertheless i think this test might break randomly. (im surprised it works tbh). assert_unordered_equality exists because postgres will return items whichever way is the fastest. That means the same query might not always return a result in consistent order. That means there might actually not be a "correct order"...

ixmp4/core/optimization/scalar.py Outdated Show resolved Hide resolved
tests/data/test_optimization_indexset.py Show resolved Hide resolved
tests/data/test_optimization_scalar.py Show resolved Hide resolved
@glatterf42 glatterf42 force-pushed the include/optimization-scalar branch from aae2d72 to 6f1fe8e Compare December 1, 2023 11:53
@glatterf42

This comment was marked as outdated.

@meksor

This comment was marked as resolved.

@meksor

This comment was marked as outdated.

@glatterf42

This comment was marked as resolved.

@glatterf42

This comment was marked as outdated.

@glatterf42 glatterf42 force-pushed the include/optimization-scalar branch 2 times, most recently from 316d9c7 to 692cab5 Compare January 11, 2024 12:06
@glatterf42

This comment was marked as outdated.

@danielhuppmann
Copy link
Member

Thanks @glatterf42 for rebasing and updating the PR. I think it's fine either way, whether the dimensionless unit comes pre-defined in a blank database or hast to be defined by a user. Whatever you prefer.

But I still think that the keyword argument should be unit instead of unit_or_name and it should take a ixmp4.Unit or a string. I believe that an argument unit_or_name creates more confusion than an argument unit that can handle multiple "forms" of a unit object.

@glatterf42
Copy link
Member Author

Thanks, then I'll keep the dimensionless unit as pre-defined. The syntax changed now so that you can supply Scalars with unit, which can either be a str or a Unit object.

@meksor, please update your review as all your points should be addressed by now. The last thing from my side is adding another DB migration.

@glatterf42
Copy link
Member Author

In fact, I don't think we need another migration here. There were some commits after d66a0a4 that affected data/db/optimization/scalar/model.py, but none that affected it in a way that changed the actual data model. When I ran alembic revision -m "Update optimization_scalar table" --autogenerate right now, it created a migration with an almost empty file, like

def upgrade():
    pass

def downgrade():
    pass

So I figure we don't need to push that migration here.

@glatterf42 glatterf42 force-pushed the include/optimization-scalar branch from d9bd7ee to ad5d2ac Compare January 22, 2024 13:06
@glatterf42
Copy link
Member Author

Rebased onto latest main.

@glatterf42 glatterf42 requested a review from meksor January 23, 2024 08:05
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.

Looks good thanks! Sorry for the delay!

ixmp4/core/optimization/scalar.py Outdated Show resolved Hide resolved
ixmp4/core/optimization/scalar.py Outdated Show resolved Hide resolved
tests/core/test_indexset.py Show resolved Hide resolved
indexset_2 = run.optimization.IndexSet("Indexset 2")
exp = df_from_list(indexsets=[indexset_1, indexset_2])
res = run.optimization.indexsets.tabulate()
# utils.assert_unordered_equality doesn't like lists, so make sure the order in
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'm kind of confused by that. Nevertheless i think this test might break randomly. (im surprised it works tbh). assert_unordered_equality exists because postgres will return items whichever way is the fastest. That means the same query might not always return a result in consistent order. That means there might actually not be a "correct order"...

@glatterf42 glatterf42 merged commit 7798462 into main Jan 25, 2024
4 checks passed
@glatterf42 glatterf42 deleted the include/optimization-scalar branch January 25, 2024 07:32
glatterf42 added a commit that referenced this pull request Jan 16, 2025
* Add data-tests for IndexSets

* Add get_by_id for Units

* Update/expand IndexSet tests

* Introduce Optimization.Scalar

* Correct Indexset UniqueConstraint and streamline core syntax

* Update Optimization.Scalar as requested

* Introduce update() function
* Call attention to that when trying to create existing Scalar
* Fix DB UniqueConstraint

* Refine Optimization.Scalar facade syntax

* Allow specifying Unit via its name
* Reflect setting Scalar.value and Scalar.unit in DB

* Clean up leftover comments

* Fix scalar.value core setter

* Include DB migration

* Update/expand IndexSet tests

* Introduce Optimization.Scalar

* Correct Indexset UniqueConstraint and streamline core syntax

* Update Optimization.Scalar as requested

* Introduce update() function
* Call attention to that when trying to create existing Scalar
* Fix DB UniqueConstraint

* Fix forgotten pytest.raises() in scalar tests

* Refine Optimization.Scalar facade syntax

* Allow specifying Unit via its name
* Reflect setting Scalar.value and Scalar.unit in DB

* Clean up leftover comments

* Fix scalar.value core setter

* Implement suggestions in data layer

* Update tests according to suggestions

* adjust ScalarRepository.update

* adjust tests

* Allow passing in Unit, unit.name or None for Scalar creation

* Reduce ruff line-length, enable checking tests/

* Adapt long comment lines to new ruff line length

* Converge IndexSet and Scalar get and create methods

* Reapply formatting lost during rebase

* Use updated syntax for run creation

* Update black,ruff versions for pre-commit

* Delete accidental blank line

* Rename unit_or_name to name

* Remove unused imports

---------

Co-authored-by: Max Wolschlager <[email protected]>
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.

3 participants