Extend API to support dynamic time slices #264

zikolach · 2020-02-04T16:26:05Z

Requirements

Changelist:

new method to add time slices to DB
new method to get list of time slices from DB
input/output dataframes containing timeseries data now may include subannual column (containing name of time slice)
relevant changes in ixmp.jar (source diff https://github.com/iiasa/ixmp_source/compare/0cd6eb355af454e3c7d214d0e5612ea455aff9c0...10742197121eaf83c6f222f969d7e2842d0bbdde)
update tests suite

Actions:

Tests added.
Documentation added.
Release notes updated.
Update this PR with ixmp.jar built from ixmp_source master

Related PR https://github.com/iiasa/ixmp_source/pull/264

tests/test_timesteps.py

ixmp/core.py

ixmp/backend/base.py

zikolach · 2020-02-05T15:35:09Z

@danielhuppmann updated based on your suggestions. Could you please help updating documentation (see checkbox in PR description)?

codecov · 2020-02-05T16:31:05Z

Codecov Report

Merging #264 into master will increase coverage by 0.10%.
The diff coverage is 98.72%.

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   96.96%   97.07%   +0.10%     
==========================================
  Files          38       38              
  Lines        3530     3760     +230     
==========================================
+ Hits         3423     3650     +227     
- Misses        107      110       +3

Impacted Files	Coverage Δ
ixmp/backend/__init__.py	`100.00% <ø> (ø)`
ixmp/testing.py	`93.91% <ø> (ø)`
ixmp/tests/test_cli.py	`100.00% <ø> (ø)`
ixmp/core.py	`93.30% <89.18%> (-0.39%)`	⬇️
ixmp/tests/core/test_timeseries.py	`99.68% <99.66%> (+0.38%)`	⬆️
ixmp/backend/base.py	`98.55% <100.00%> (+0.04%)`	⬆️
ixmp/backend/jdbc.py	`94.87% <100.00%> (+0.05%)`	⬆️
ixmp/tests/backend/test_base.py	`98.27% <100.00%> (+0.06%)`	⬆️
ixmp/tests/core/test_platform.py	`100.00% <100.00%> (ø)`
ixmp/utils.py	`95.91% <100.00%> (+0.14%)`	⬆️
... and 1 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 9a2f0fe...bff8df0. Read the comment docs.

khaeru · 2020-02-07T10:02:59Z

ixmp/core.py

@@ -286,6 +286,31 @@ def add_region_synonym(self, region, mapped_to):

        self._backend.set_node(region, synonym=mapped_to)

+    def timeslices(self):
+        """Return all subannual time slices defined for the IAMC-style


Please follow the numpydoc standard: the short summary should fit on one line.

ixmp/core.py

tests/test_timeslices.py

khaeru · 2020-02-07T10:07:29Z

Documentation added.

One thing I would like to see described: what is the meaning of time slice ‘categories’? Do they have any functional effect? How can they be defined?

zikolach · 2020-02-07T14:15:59Z

One thing I would like to see described: what is the meaning of time slice ‘categories’? Do they have any functional effect? How can they be defined?

From my understanding, time attribute of time series defines to which particular time span time series data belong to. For e.g. to distinguish summer/winter indicator values. But I would prefer @danielhuppmann to formulate the description for documentation as I may have not complete understanding of the purpose or intentions.

khaeru · 2020-02-27T10:31:13Z

FYI, #267 incidentally fixed some CI issues that had come up. Rebasing this PR will help the checks pass.

ixmp/backend/base.py

khaeru · 2020-03-03T12:54:14Z

In #269, @danielhuppmann wrote:

We have an unresolvable naming conflict between MESSAGE (where time is the set of subannual time periods) and the IAMC format, where time is used by openscm (and therefore pyam) for continuous-time format.

I think in this case I have totally failed to understand what the data model is supposed to be—and I thought I had been paying attention/working with the code. New users will be even more confused.

This points to the need for a clear description of the various data structure(s) that are supported and the semantics of how they are represented.

khaeru · 2020-03-10T14:55:25Z

@zikolach I just said to @danielhuppmann on Slack that, because of the adjustments throughout the test suite in #270, this will need a rebase. If it's not obvious how to resolve the conflicts, please ask and I can help.

zikolach · 2020-03-11T12:57:15Z

FYI, #267 incidentally fixed some CI issues that had come up. Rebasing this PR will help the checks pass.

Tests passed on TC so I pushed changes to original branch. Please have a look as it might be some leftovers in test_timeseries.py after merging timeseries-related tests in one file.

@zikolach

- allow to add timesteps to db - get list of timesteps from db - update dockstrings - update release notes - add support for adding and retrieving of timeseries having time dimension - add python3-setuptools to test image Dockerfile - add docstrings related to the `timeslices` feature - appease stickler, harmonize notation - be consistent about upper/lower-case (per review comment by @zikolach) - implement review comments by @khaeru - add docstrings on relation of timeslice and message_ix.Scenario 'time' - exclude JPype1 version 0.7.2 due to crashes - add tests and rename time to subannual (#277) - add check for duplicate timeslice addition (with unit test) - refactor to names similar to `pyam` for consistency - extend test for subannual timeseries data - update timeslices API - fix error message produced by add_timeslice (+ fixed test) - fix check_out method called on TimeSeries object - update test to use time (instead of subannual) name (rename will be addressed separately) - update timeslices API - rename time to subannual in "public facing API" - return from java timeseries time attribute as subannual Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>

- adjust test to in-memory db with dynamic population - move tests to ixmp/tests - add subannual to test fixtures - add missing sparse

zikolach · 2020-03-17T17:48:29Z

@danielhuppmann do you plan to add more tests or change anything in this branch?

If not I would go ahead and:

merge iiasa/ixmp_source#264
update jar in this PR and merge PR into master

danielhuppmann · 2020-03-18T06:40:00Z

@danielhuppmann do you plan to add more tests or change anything in this branch?

No, I don't see any additional tests necessary (for the new features) - good to go as far as I'm concerned.

danielhuppmann

thanks @zikolach!

zikolach · 2020-03-18T08:40:27Z

@danielhuppmann I am going to merge ixmp and commit fresh jar build from master to this PR

khaeru · 2020-03-18T08:45:31Z

Can we have a complete description of this PR, for future reference?

zikolach · 2020-03-18T09:06:32Z

Can we have a complete description of this PR, for future reference?

Sure, I'll update it

- source diff https://github.com/iiasa/ixmp_source/compare/0cd6eb355af454e3c7d214d0e5612ea455aff9c0...10742197121eaf83c6f222f969d7e2842d0bbdde

khaeru

@zikolach @danielhuppmann thanks for the efforts here, it's looking good! A number of minor changes; the major one is to please try to follow the style of the existing tests in terms of re-using data. This makes it much easier to focus on what is being tested in each test, rather than having to skim past many lines that redefine the same data.

If I can supply some text, e.g. for a comment that provides a guide to this style, please ask.

ixmp/backend/base.py

ixmp/backend/jdbc.py

ixmp/core.py

ixmp/tests/core/test_timeseries.py

ixmp/tests/test_timeslices.py

ixmp/utils.py

…always presented

zikolach · 2020-03-18T13:16:21Z

@khaeru thanks for great (detailed) feedback! I followed your recommendations of updating docstrings and tests.

- move adding subannual column to to_iamc_template - simplify method implementation - add missing subannual parameter to remove_timeseries/backend

- Broken by iiasa/ixmp#264. - Fixed by iiasa/ixmp#295.

In #270, this file was cleaned up and tests were collected in a class, with parametrization. Shortly after, #264 was merged, which squashed these improvements. This commit rescues the improvements and adapts additions since then to the same pattern.

zikolach requested a review from danielhuppmann February 5, 2020 08:28

zikolach self-assigned this Feb 5, 2020

danielhuppmann reviewed Feb 5, 2020

View reviewed changes

tests/test_timesteps.py Outdated Show resolved Hide resolved

danielhuppmann reviewed Feb 5, 2020

View reviewed changes

ixmp/core.py Outdated Show resolved Hide resolved

danielhuppmann reviewed Feb 5, 2020

View reviewed changes

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

danielhuppmann reviewed Feb 5, 2020

View reviewed changes

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

zikolach changed the title ~~Replace timespans with timesteps~~ Replace timespans with timeslices Feb 5, 2020

zikolach requested a review from danielhuppmann February 5, 2020 15:34

zikolach marked this pull request as ready for review February 5, 2020 20:39

zikolach requested a review from khaeru February 6, 2020 09:44

khaeru reviewed Feb 7, 2020

View reviewed changes

ixmp/core.py Show resolved Hide resolved

khaeru reviewed Feb 7, 2020

View reviewed changes

tests/test_timeslices.py Outdated Show resolved Hide resolved

danielhuppmann reviewed Feb 28, 2020

View reviewed changes

ixmp/backend/base.py Show resolved Hide resolved

danielhuppmann mentioned this pull request Feb 28, 2020

add docstrings related to the timeslices feature #269

Merged

zikolach force-pushed the feature/time-steps branch from dcb6fc1 to 13e51d4 Compare February 28, 2020 13:02

khaeru mentioned this pull request Mar 5, 2020

Harmonize handling of codelists #274

Open

zikolach mentioned this pull request Mar 10, 2020

Add tests and rename time to subannual #277

Merged

zikolach and others added 3 commits March 11, 2020 15:29

Fix tests

26472e3

- adjust test to in-memory db with dynamic population - move tests to ixmp/tests - add subannual to test fixtures - add missing sparse

Combine all timeseries tests in core/test_timeseries.py

6ee8977

zikolach force-pushed the feature/time-steps branch from 7f6170d to 6ee8977 Compare March 11, 2020 14:34

zikolach and others added 2 commits March 11, 2020 15:36

Fix stickler warning

dbbe3a0

add warning if importing timeseries with unexpected columns (#280)

f7822d3

danielhuppmann self-requested a review March 18, 2020 06:40

danielhuppmann approved these changes Mar 18, 2020

View reviewed changes

zikolach added 2 commits March 18, 2020 12:06

Updated ixmp.jar

3b25c7d

- source diff https://github.com/iiasa/ixmp_source/compare/0cd6eb355af454e3c7d214d0e5612ea455aff9c0...10742197121eaf83c6f222f969d7e2842d0bbdde

Fix export timeseries test

e0543cd

zikolach changed the title ~~Replace timespans with timeslices~~ Extend API to support dynamic time slices Mar 18, 2020

khaeru requested changes Mar 18, 2020

View reviewed changes

zikolach added 5 commits March 18, 2020 13:21

Minor changes in docstrings and parameter names

0f9a2cc

Move time slices tests to platform

602378b

Remove check for subannual column in timeseries data as it should be …

b8d49a5

…always presented

Reuse/move fixtures to DATA

6877b5f

Fix linter warnings + reuse test fixtures

1b400ec

zikolach added 3 commits March 18, 2020 16:18

Update add_timeseries/remove_timeseries

5f6b89c

- move adding subannual column to to_iamc_template - simplify method implementation - add missing subannual parameter to remove_timeseries/backend

Minor docstring update

4d3bce2

Minor docstring update

bff8df0

khaeru approved these changes Mar 19, 2020

View reviewed changes

zikolach merged commit e66f3f8 into master Mar 19, 2020

zikolach deleted the feature/time-steps branch March 19, 2020 10:06

khaeru mentioned this pull request Mar 21, 2020

Update tests for subannual time slices iiasa/message_ix#322

Closed

khaeru added a commit to khaeru/message_ix that referenced this pull request May 8, 2020

Un-xfail integration tests (iiasa#322)

31944a4

- Broken by iiasa/ixmp#264. - Fixed by iiasa/ixmp#295.

khaeru mentioned this pull request May 25, 2021

Improve testing for v3.3; misc cleanup #416

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend API to support dynamic time slices #264

Extend API to support dynamic time slices #264

zikolach commented Feb 4, 2020 •

edited

zikolach commented Feb 5, 2020

codecov bot commented Feb 5, 2020 •

edited

khaeru Feb 7, 2020

khaeru commented Feb 7, 2020

zikolach commented Feb 7, 2020 •

edited

khaeru commented Feb 27, 2020

khaeru commented Mar 3, 2020 •

edited

khaeru commented Mar 10, 2020

zikolach commented Mar 11, 2020 •

edited

zikolach commented Mar 17, 2020

danielhuppmann commented Mar 18, 2020

danielhuppmann left a comment

zikolach commented Mar 18, 2020

khaeru commented Mar 18, 2020

zikolach commented Mar 18, 2020

khaeru left a comment

zikolach commented Mar 18, 2020

Extend API to support dynamic time slices #264

Extend API to support dynamic time slices #264

Conversation

zikolach commented Feb 4, 2020 • edited

zikolach commented Feb 5, 2020

codecov bot commented Feb 5, 2020 • edited

Codecov Report

khaeru Feb 7, 2020

Choose a reason for hiding this comment

khaeru commented Feb 7, 2020

zikolach commented Feb 7, 2020 • edited

khaeru commented Feb 27, 2020

khaeru commented Mar 3, 2020 • edited

khaeru commented Mar 10, 2020

zikolach commented Mar 11, 2020 • edited

zikolach commented Mar 17, 2020

danielhuppmann commented Mar 18, 2020

danielhuppmann left a comment

Choose a reason for hiding this comment

zikolach commented Mar 18, 2020

khaeru commented Mar 18, 2020

zikolach commented Mar 18, 2020

khaeru left a comment

Choose a reason for hiding this comment

zikolach commented Mar 18, 2020

zikolach commented Feb 4, 2020 •

edited

codecov bot commented Feb 5, 2020 •

edited

zikolach commented Feb 7, 2020 •

edited

khaeru commented Mar 3, 2020 •

edited

zikolach commented Mar 11, 2020 •

edited