Meta support for model/scenario levels #353

zikolach · 2020-07-28T12:32:56Z

Implement full meta functionality.

Implement full meta functionality on the Platform level, and make some adaptions to the Scenario meta functionality too. Also, add the possibility of creating a model or scenario on Platform. This PR relies on https://github.com/iiasa/ixmp_source/pull/323.

Detailed list of changes:

add Platform.add_model using Backend.add_model
add Platform.add_scenario using Backend.add_scenario
add Platform.models using Backend.models
add Platform.scenarios using Backend.scenarios
add Platform.get_meta using Backend.get_meta
add Platform.set_meta using Backend.set_meta
add Platform.remove_meta using Backend.remove_meta
add Scenario.remove_meta using Backend.remove_scenario_meta
deprecate Scenario.delete_meta

How to review

make sure the tests pass
check that the built sphinx documentation is how you'd expect it

Problematic things relevant to this PR:

ambiguous use of the name Scenario. As an ixmp.Scenario is actually an ixmp_source run that has a scenario itself, distinguishing between these two types of Scenarios is hard. Hence Platform.scenarios() and Platform.scenario_list() are ambigous.

PR checklist

Tests added.
Documentation added.
Release notes updated.

zikolach · 2020-07-28T12:34:12Z

ixmp/core.py

@@ -393,6 +393,39 @@ def check_access(self, user, models, access='view'):
        else:
            return {model: result.get(model) == 1 for model in models_list}

+    def get_meta(self, *args, **kwargs):


we can avoid this redundant calls by adding get_meta and set_meta to _backend_direct list

see this commit 121a876

zikolach · 2020-07-29T08:08:22Z

ixmp/backend/jdbc.py

+                 version = None):
+        if not (model or scenario or version):
+            msg = ('At least one parameter has to be provided out of: '
+                   'model, scenario, version')


I don't think passing just version makes much sense. It should be one of:

model

scenario

model + scenario

model + scenario + version

zikolach · 2020-07-29T08:13:53Z

ixmp/tests/core/test_meta.py

+from ixmp.testing import models
+
+
+sample_meta = {'sample_string': 3, 'another_string': 'string_value'}


first entry has not a string value. What is the idea? Maybe just called by mistake

zikolach · 2020-07-30T11:08:12Z

ixmp/backend/base.py

@@ -902,8 +1020,8 @@ def get_meta(self, s: Scenario):
        """

    @abstractmethod
-    def set_meta(self, s: Scenario, name_or_dict, value=None):
-        """Set single or multiple meta entries.
+    def set_scenario_meta(self, s: Scenario, name_or_dict, value=None):


seems to be duplicated after renaming

with that one https://github.com/iiasa/ixmp/pull/353/files#diff-313b0713bfcf3262902e8b94639dd49eR986

khaeru · 2020-08-03T12:14:12Z

I'll wait to review until the tests pass and conflicts are resolved.

In the meantime, some questions:

In addition to worsening (with add_scenario()) the existing overlap between the semantics of scenario names and Scenario objects (as the description notes), this PR creates a new overlap between model names (add_model()) and the existing Model class. Can this be avoided, e.g. perhaps via the approach described in Harmonize handling of codelists #274?
What is the intended behaviour if a user calls add_timeseries with model name(s) or scenario name(s) that have not previously been declared with add_model or add_scenario?
What is the intended behaviour if a user instantiates ixmp.Scenario(..., version='new') with a model name or scenario name that have not been previously declared?

zikolach · 2020-08-04T12:30:20Z

What is the intended behaviour if a user instantiates ixmp.Scenario(..., version='new') with a model name or scenario name that have not been previously declared?

This behavior did not change - it will create (register) new model/scenario names. Methods were added to allow to create entities which rely on specific model/scenario names before scenario version (run) is created.

zikolach · 2020-08-04T12:31:43Z

What is the intended behaviour if a user calls add_timeseries with model name(s) or scenario name(s) that have not previously been declared with add_model or add_scenario?

Not sure how does it relate to add_timeseries. Can you please point on the code example or explain what you mean?

zikolach · 2020-08-04T12:44:58Z

Can this be avoided, e.g. perhaps via the approach described in #274?

I think it makes sense to apply this paradigm of codelists in longer-term perspective. Add mode/scenario names methods are implemented in line with current (probably not efficient) aproach. So later on we can change all such occurrences as adding regions, timeslices, units and new model/scennario names to more generic API in one go.

To allow for the generic get_meta, set_meta, delete_meta methods, rename the scenario specific meta methods.

- update ixmp.jar - add Platform.get_meta, Platform.set_meta - add some tests (only two pass so far) TODO: make all tests pass, maybe add more tests

- remove redundant backend methods from platform - use unwrap to handle BigDecimals - fix tests - replace ixmp.jar (freshly built from appropriate branch)

- look for more specific errors - add new test for Model+Scenario Meta

- set_meta void return type - set meta version parameter type

- implement Platform.remove_meta - deprecate Scenario.delete_meta (Scenario.remove_meta should be used) - add more tests

- update jar

- add tests

- add release notes - update docstrings - remove unused backend method delete_scenario_meta

describe valid args of getting/setting/deleting meta indicators

- source diff https://github.com/iiasa/ixmp_source/compare/2c3244517d6ffd6de70c896d3198d9d3a43ddfae...07b82efe90a6590b30cc764804f1e9187e9cfb55

codecov · 2020-08-06T12:47:09Z

Codecov Report

Merging #353 into master will increase coverage by 0.24%.
The diff coverage is 99.39%.

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
+ Coverage   96.26%   96.50%   +0.24%     
==========================================
  Files          44       45       +1     
  Lines        4843     5100     +257     
==========================================
+ Hits         4662     4922     +260     
+ Misses        181      178       -3

Impacted Files	Coverage Δ
ixmp/core.py	`95.10% <85.71%> (-0.36%)`	⬇️
ixmp/backend/base.py	`98.81% <100.00%> (+0.06%)`	⬆️
ixmp/backend/jdbc.py	`95.84% <100.00%> (+0.68%)`	⬆️
ixmp/tests/backend/test_base.py	`98.66% <100.00%> (+0.09%)`	⬆️
ixmp/tests/core/test_meta.py	`100.00% <100.00%> (ø)`
ixmp/tests/core/test_platform.py	`100.00% <100.00%> (ø)`
ixmp/tests/core/test_scenario.py	`100.00% <100.00%> (ø)`
ixmp/backend/io.py	`98.54% <0.00%> (+0.01%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44f3bd2...8f78a8c. Read the comment docs.

fonfon · 2020-08-06T14:14:18Z

This PR is now ready to be reviewed.

'strict' will only retrieve meta indicators from the exact given level (model, scenario, version combination).

khaeru

@zikolach, thanks for responses above, and @fonfon for the code additions.

Required changes:

I agree with @zikolach that set_scenario_meta and remove_scenario_meta duplicate set_meta and remove_meta, respectively. Please delete the former two. If the Java code has multiple methods/functions, then JDBCBackend.set_meta must choose the correct one to call.
For us to avoid confusing ourselves, please rename the new methods added to the Backend API as follows:
- add_model → add_model_name.
- add_scenario → add_scenario_name.
- models → get_model_names (this aligns with the existing get_units. Backend method names should have a verb for clarity.)
- scenarios → get_scenario_names
Ensure method implementations in JDBCBackend are in exactly the same order as base.Backend.

ixmp/backend/base.py

ixmp/core.py

khaeru · 2020-08-12T09:30:45Z

General responses to @zikolach's comments:

This behavior did not change - it will create (register) new model/scenario names. Methods were added to allow to create entities which rely on specific model/scenario names before scenario version (run) is created.

The issue I was trying to highlight was that the existing behaviour was never clearly explained or documented; it was only implicit. Namely, it's not explained to the user anywhere that:

Model and scenario names are not simply attributes of TimeSeries/Scenario objects, but distinct entities in the data model, to which things (e.g. annotations/meta) can be attached.
These entities were automatically created when new values were used; now they can be created directly.

I think it makes sense to apply this paradigm of codelists in longer-term perspective. […] So later on we can change all such occurrences as adding regions, timeslices, units and new model/scennario names to more generic API in one go.

Totally agree with this. I think it will also be much easier to explain to the user via documentation, etc.: "ixmp tracks lists of strings or codes that are used for different purposes, and allows attaching meta information to these codes."

In any case, let's merge this PR once the above changes are made, then clean up the Backend API on the Python side in a separate PR, soon. The Java implementation could be simplified later, when time allows.

- to distinguish from Scenario/Model objects, rename methods to get/retrieve scenarios/models to scenario_name and model_name - adapt documentation of parameters

- Adapt to renamed method names - some minor documentation fixes/updates

zikolach · 2020-08-17T07:37:10Z

@khaeru seems like all the requested changes are done. Could you please re-review/approve this PR?

- update release notes to changed method names - remove methods from backend test

- correct method name to get_model_names() from model_names()

khaeru · 2020-08-17T08:16:38Z

seems like all the requested changes are done. Could you please re-review/approve this PR?

Yep! I did see the review re-request from @fonfon a few minutes before your comment—also three commits since, but I guess those are the final ones. I will review as soon as I have time.

khaeru

Thanks for making the requested changes, @fonfon.

There are some minor typos and other issues, but since a lot of this code will be simplified or removed when we make the changes described at #353 (comment), they can go in for now.

I will squash and merge.

Missed by #353 and #366

Per #296. This was missed in #353.

zikolach commented Jul 28, 2020

View reviewed changes

zikolach assigned fonfon Jul 28, 2020

zikolach changed the title ~~Feature/meta support~~ Meta support for model/scenario levels Jul 28, 2020

zikolach commented Jul 29, 2020

View reviewed changes

fonfon force-pushed the feature/meta-support branch from 1d81c13 to af481d0 Compare July 29, 2020 15:36

zikolach commented Jul 30, 2020

View reviewed changes

fonfon force-pushed the feature/meta-support branch from d0e4227 to de61b9b Compare August 1, 2020 09:09

fonfon marked this pull request as ready for review August 1, 2020 09:52

fonfon requested a review from khaeru August 1, 2020 09:52

fonfon marked this pull request as draft August 1, 2020 09:55

fonfon and others added 15 commits August 6, 2020 14:15

Meta: add first dummy methods and tests

8b486c2

Meta: rename scenario meta methods

beecfe6

To allow for the generic get_meta, set_meta, delete_meta methods, rename the scenario specific meta methods.

Meta: add Platform method to get/set meta

e4e5ba1

- update ixmp.jar - add Platform.get_meta, Platform.set_meta - add some tests (only two pass so far) TODO: make all tests pass, maybe add more tests

Minor changes

9f5c59e

- remove redundant backend methods from platform - use unwrap to handle BigDecimals - fix tests - replace ixmp.jar (freshly built from appropriate branch)

Update Meta tests

05e29ee

- look for more specific errors - add new test for Model+Scenario Meta

Update ixmp.jar

24e142c

- set_meta void return type - set meta version parameter type

Meta: Implement Platform.remove meta; add tests

f602c39

- implement Platform.remove_meta - deprecate Scenario.delete_meta (Scenario.remove_meta should be used) - add more tests

Meta: improve validation of arguments

84d1677

Meta: add tests setting/retrieving booleans

0fc7407

Meta: improve implementation, test scenario meta args

41e34ff

Expose adding model/scenario names

afe9657

- update jar

Meta: add more tests

db6df29

Create abstract base methods for adding model/scenario

9032806

Add models/scenarios list methods

3054d37

- add tests

Meta: add release notes, update documentation

0a48369

- add release notes - update docstrings - remove unused backend method delete_scenario_meta

Meta documentation update: valid args

9ba12e3

describe valid args of getting/setting/deleting meta indicators

fonfon force-pushed the feature/meta-support branch from b2eafd5 to 9ba12e3 Compare August 6, 2020 12:24

fonfon added 2 commits August 6, 2020 14:35

Updated ixmp.jar

7699af5

- source diff https://github.com/iiasa/ixmp_source/compare/2c3244517d6ffd6de70c896d3198d9d3a43ddfae...07b82efe90a6590b30cc764804f1e9187e9cfb55

Flake8 fix: unused f-string

832654f

fonfon marked this pull request as ready for review August 6, 2020 14:14

Platform.get_meta: add 'strict' parameter

6205c42

'strict' will only retrieve meta indicators from the exact given level (model, scenario, version combination).

fonfon force-pushed the feature/meta-support branch from 58354a6 to 6205c42 Compare August 10, 2020 14:35

khaeru requested changes Aug 12, 2020

View reviewed changes

ixmp/backend/base.py Outdated Show resolved Hide resolved

ixmp/core.py Show resolved Hide resolved

fonfon added 6 commits August 13, 2020 18:38

Remove calls of get|set|remove_scenario_meta

9c03359

Order backend jdbc and base methods.

004fa85

Adapt changed error message.

ff03ede

Rename scenario/model methods

84ffa4a

- to distinguish from Scenario/Model objects, rename methods to get/retrieve scenarios/models to scenario_name and model_name - adapt documentation of parameters

Update documentation.

1f7cba3

- Adapt to renamed method names - some minor documentation fixes/updates

Fix minor flake8 syntax error.

e7c48ee

fonfon requested a review from khaeru August 17, 2020 07:24

fonfon added 3 commits August 17, 2020 09:39

Update release notes and backend test

a939e1e

- update release notes to changed method names - remove methods from backend test

Order backend test methods alphabetically.

43a45a5

Minor release notes adaption.

8f78a8c

- correct method name to get_model_names() from model_names()

khaeru approved these changes Aug 17, 2020

View reviewed changes

khaeru merged commit 9326f3b into master Aug 17, 2020

khaeru mentioned this pull request Aug 17, 2020

Harmonize handling of codelists #274

Open

zikolach deleted the feature/meta-support branch August 26, 2020 09:02

khaeru added a commit that referenced this pull request Aug 26, 2020

Improve description of #353 in release notes

b1efdaf

khaeru added a commit that referenced this pull request Oct 26, 2020

Squash warning due to deprecated use of delete_meta() in test_meta()

eff4cd6

Missed by #353 and #366

khaeru added a commit that referenced this pull request May 25, 2021

Convert Java to Python exceptions in {gs}et_meta()

fbe1ac6

Per #296. This was missed in #353.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meta support for model/scenario levels #353

Meta support for model/scenario levels #353

zikolach commented Jul 28, 2020 •

edited by fonfon

Loading

zikolach Jul 28, 2020

zikolach Jul 28, 2020

zikolach Jul 29, 2020

zikolach Jul 29, 2020

zikolach Jul 30, 2020 •

edited

Loading

zikolach Jul 30, 2020

khaeru commented Aug 3, 2020 •

edited

Loading

zikolach commented Aug 4, 2020

zikolach commented Aug 4, 2020

zikolach commented Aug 4, 2020

codecov bot commented Aug 6, 2020 •

edited

Loading

fonfon commented Aug 6, 2020 •

edited

Loading

khaeru left a comment

khaeru commented Aug 12, 2020

zikolach commented Aug 17, 2020

khaeru commented Aug 17, 2020

khaeru left a comment

		from ixmp.testing import models


		sample_meta = {'sample_string': 3, 'another_string': 'string_value'}

Meta support for model/scenario levels #353

Meta support for model/scenario levels #353

Conversation

zikolach commented Jul 28, 2020 • edited by fonfon Loading

How to review

PR checklist

zikolach Jul 28, 2020

Choose a reason for hiding this comment

zikolach Jul 28, 2020

Choose a reason for hiding this comment

zikolach Jul 29, 2020

Choose a reason for hiding this comment

zikolach Jul 29, 2020

Choose a reason for hiding this comment

zikolach Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

zikolach Jul 30, 2020

Choose a reason for hiding this comment

khaeru commented Aug 3, 2020 • edited Loading

zikolach commented Aug 4, 2020

zikolach commented Aug 4, 2020

zikolach commented Aug 4, 2020

codecov bot commented Aug 6, 2020 • edited Loading

Codecov Report

fonfon commented Aug 6, 2020 • edited Loading

khaeru left a comment

Choose a reason for hiding this comment

khaeru commented Aug 12, 2020

zikolach commented Aug 17, 2020

khaeru commented Aug 17, 2020

khaeru left a comment

Choose a reason for hiding this comment

zikolach commented Jul 28, 2020 •

edited by fonfon

Loading

zikolach Jul 30, 2020 •

edited

Loading

khaeru commented Aug 3, 2020 •

edited

Loading

codecov bot commented Aug 6, 2020 •

edited

Loading

fonfon commented Aug 6, 2020 •

edited

Loading