Closed zikolach closed 4 years ago
I'll wait to review until the tests pass and conflicts are resolved.
In the meantime, some questions:
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 #274?add_timeseries
with model name(s) or scenario name(s) that have not previously been declared with add_model
or add_scenario
?ixmp.Scenario(..., version='new')
with a model name or scenario name that have not been previously declared?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.
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?
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.
Merging #353 into master will increase coverage by
0.24%
. The diff coverage is99.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%) |
:arrow_down: |
ixmp/backend/base.py | 98.81% <100.00%> (+0.06%) |
:arrow_up: |
ixmp/backend/jdbc.py | 95.84% <100.00%> (+0.68%) |
:arrow_up: |
ixmp/tests/backend/test_base.py | 98.66% <100.00%> (+0.09%) |
:arrow_up: |
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%) |
:arrow_up: |
... 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.
This PR is now ready to be reviewed.
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:
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.
@khaeru seems like all the requested changes are done. Could you please re-review/approve this PR?
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.
Implement full meta functionality.
Implement full meta functionality on the
Platform
level, and make some adaptions to theScenario
meta functionality too. Also, add the possibility of creating a model or scenario onPlatform
. This PR relies on https://github.com/iiasa/ixmp_source/pull/323.Detailed list of changes:
Platform.add_model
usingBackend.add_model
Platform.add_scenario
usingBackend.add_scenario
Platform.models
usingBackend.models
Platform.scenarios
usingBackend.scenarios
Platform.get_meta
usingBackend.get_meta
Platform.set_meta
usingBackend.set_meta
Platform.remove_meta
usingBackend.remove_meta
Scenario.remove_meta
usingBackend.remove_scenario_meta
Scenario.delete_meta
How to review
Problematic things relevant to this PR:
Scenario
. As anixmp.Scenario
is actually an ixmp_sourcerun
that has ascenario
itself, distinguishing between these two types of Scenarios is hard. HencePlatform.scenarios()
andPlatform.scenario_list()
are ambigous.PR checklist