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

Transfer Excel data I/O from message_ix #289

Merged
merged 44 commits into from
Mar 25, 2020
Merged

Transfer Excel data I/O from message_ix #289

merged 44 commits into from
Mar 25, 2020

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Mar 23, 2020

This PR streamlines and improves file input/output in the MESSAGE stack:

  • Scenario.read_excel(), .to_excel() are brought to ixmp from message_ix (see Transfer Excel data I/O to ixmp message_ix#321).
    • Can now be used with any ixmp Scenario.
    • The code is improved in several ways, e.g. it can now distinguish between 1-D index sets and indexed sets; degrades gracefully by giving readable log messages and exceptions.
  • TimeSeries.read_file is added as a replacement for ixmp.utils.import_timeseries.
  • New: commands import and export that allow performing the above operations from the command-line.
  • New: documentation that clearly describes the file formats supported (and not supported) for I/O; docstrings for all touched methods.
  • New: TimeSeries.add_timeseries(..., year_lim=(2020, 2080)), which brings the minimum/maximum year functionality previously only supported by import_timeseries() to non-file-based operations.
  • See Try to fix failing build on AppVeyor #290 & thanks @zikolach for:
    • ixmp.jar is updated to clean up Java exception handling.
    • {,JDBC}Backend gain a method, get_log_level().

Closes #284.
Replaces #286.

How to review

  • Build and read the documentation.

  • Check that any I/O scripts still work.

    One possible breaking change: import_timeseries() previously automatically prepended the string 'R11_' to imported region names, unless the name was 'World'. This is specific to MESSAGEix-GLOBIOM (not message_ix), so it is removed.

PR checklist

  • Tests added.
  • Documentation added.
  • Release notes updated.

- TimeSeries.read_file: new method.
- TimeSeries.add_timeseries: add a `year_lim` argument, so limiting
  data by year can be done even when not reading from file.
- core.to_iamc_template: rename to core.to_iamc_layout, update
  docstring, remove duplicate set computation.
- Simplify cli.import_timeseries.
- Update docs.
@khaeru khaeru added the enh New features & functionality label Mar 23, 2020
@khaeru khaeru added this to the 3.0 milestone Mar 23, 2020
khaeru and others added 5 commits March 24, 2020 16:43
- remove finally block in timeseries commit method (java)
- change input validation in setStatus method (java)
- fix commit in Scenario class (java)
- propagate exceptions from commit (java)
- upgrade pytest dependency
- add platform method to get current log level (remove changing log level in tests as it affects other tests log output)
- updated ixmp.jar (source diff iiasa/ixmp_source@1074219...923f5b9)
@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #289 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #289   +/-   ##
=======================================
  Coverage   97.38%   97.38%           
=======================================
  Files          39       39           
  Lines        3979     3979           
=======================================
  Hits         3875     3875           
  Misses        104      104           

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 386f3fe...386f3fe. Read the comment docs.

Copy link
Contributor

@zikolach zikolach left a comment

Choose a reason for hiding this comment

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

Didn't review code itself yet. Commented on docs about format.

doc/source/file-io.rst Show resolved Hide resolved
doc/source/file-io.rst Show resolved Hide resolved
doc/source/file-io.rst Show resolved Hide resolved
ixmp/backend/base.py Show resolved Hide resolved
ixmp/backend/jdbc.py Show resolved Hide resolved
ixmp/backend/jdbc.py Show resolved Hide resolved
ixmp/core.py Show resolved Hide resolved
Copy link
Contributor

@zikolach zikolach left a comment

Choose a reason for hiding this comment

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

Thanks Paul for implementing and explaining details. Overall it looks very good and as a big step forward!

I didn't test assuming new read/write of model data is covered with tests.

In general would be good to allow splitting items data into multiple sheets as we can reach rows limit. We could avoid to have separate sheet with item name-type mapping by embedding type into sheet name, e.g. type|name|n or similar convention, where type is item type, name - item name and n is a number of sheet for "large" items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer Excel data import from message_ix
3 participants