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

Improve reporting (phase 2) #126

Closed
wants to merge 82 commits into from
Closed

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Mar 1, 2019

Superseded by #144; text below may be out of date.


This will implement the second phase of the reporting update.

See also:

Progress
Address requirements from wiki:

  • A1. Provide equal support for any type of model that can be built on the ixmp.
  • A2. Support use of exogenous data.
  • A3. Support basic operations:
    • A3i. Weighted sums using model or exogenous weights.
    • A3ii. Disaggregation using model or exogenous shares.
    • A3iii. Interpolation: linear, exponential, etc.
  • A4. Support operations using quantities at different levels of aggregation.
  • A5. Support user-defined operations.
  • A6. Duplicate or clone existing operations for multiple other sets of inputs or outputs.
  • A7. Support renaming of quantities using multiple name mappings:
    • A7i. …given programmatically.
    • A7ii. …loaded from file.
  • A8. Compute only a subset of quantities from a larger/complete report.
    • A8i. …for testing/development use-cases.
    • A8ii. …for high-performance use-cases.
    • A8iii. …using command-line arguments or other straightforward inputs to specify the subset.
  • A9. Handle units for quantities.
    • Both quantities with provided units, and quantities without units.
    • Derive units for operations such as division and multiplication.
  • A10. Provide automated description of how quantities are computed, as text (console output) or image.
  • A11. Callable through retixmp (tests require Test rixmp using 'testthat' #135)

PR checklist:

  • Tests added (ongoing)
  • Documentation added — preview here.
  • Description in RELEASE_NOTES.md added

Miscellaneous:

  • Choose target release: 0.3 or 1.0?
  • Decide: this adds dependencies on dask[array] (thus toolz), graphviz (for dask.visualize), pint, and xarray. Should these be non-optional requirements, or extras (ixmp[reporting])?
  • Per @gidden comment below, provide user-friendly exception handling when bad units are encountered.

@khaeru khaeru added the enh New features & functionality label Mar 1, 2019
@khaeru khaeru self-assigned this Mar 1, 2019
tests/testing_utils.py Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
ixmp/reporting/__init__.py Outdated Show resolved Hide resolved
ixmp/reporting/__init__.py Outdated Show resolved Hide resolved
ixmp/reporting/computations.py Outdated Show resolved Hide resolved
ixmp/reporting/utils.py Outdated Show resolved Hide resolved
@khaeru khaeru mentioned this pull request Mar 7, 2019
3 tasks
@khaeru khaeru changed the title [WIP] Improve reporting (phase 2) 🚧 Improve reporting (phase 2) Mar 25, 2019
@khaeru khaeru changed the title 🚧 Improve reporting (phase 2) 🚧 Improve reporting (phase 2) Mar 25, 2019
@khaeru khaeru changed the title 🚧 Improve reporting (phase 2) WIP: Improve reporting (phase 2) Mar 25, 2019
@khaeru
Copy link
Member Author

khaeru commented Apr 2, 2019

From this point, the added dependency on xarray pulls in version 0.12, which drops support for Python 2; so Python 2 CI will fail. The last commit disables the Python 2 CI temporarily. There should probably be a separate PR that strips out all Python 2 compatibility hacks and removes the CI; this can be rebased after that's merged.

@gidden
Copy link
Member

gidden commented Apr 2, 2019 via email

@khaeru
Copy link
Member Author

khaeru commented Apr 2, 2019

First light on visualization:
visualize

ixmp/reporting/__init__.py Outdated Show resolved Hide resolved
ixmp/reporting/__init__.py Show resolved Hide resolved
ixmp/reporting/computations.py Outdated Show resolved Hide resolved
ixmp/reporting/utils.py Outdated Show resolved Hide resolved
tests/test_reporting.py Outdated Show resolved Hide resolved
tests/test_reporting.py Outdated Show resolved Hide resolved
ixmp/cli.py Outdated Show resolved Hide resolved
ixmp/reporting/computations.py Outdated Show resolved Hide resolved
ixmp/reporting/computations.py Outdated Show resolved Hide resolved
ixmp/reporting/computations.py Show resolved Hide resolved
ixmp/reporting/utils.py Show resolved Hide resolved
tests/test_reporting.py Outdated Show resolved Hide resolved
@khaeru
Copy link
Member Author

khaeru commented May 7, 2019

Updated the PR description to show the requirements (copied from the wiki). Only a few remain.

ixmp/reporting/__init__.py Outdated Show resolved Hide resolved
ixmp/reporting/__init__.py Outdated Show resolved Hide resolved
ixmp/reporting/utils.py Outdated Show resolved Hide resolved
ixmp/reporting/utils.py Show resolved Hide resolved
ixmp/reporting/utils.py Outdated Show resolved Hide resolved
ixmp/reporting/utils.py Show resolved Hide resolved
ixmp/reporting/utils.py Outdated Show resolved Hide resolved
ixmp/reporting/utils.py Outdated Show resolved Hide resolved
tests/test_reporting.py Outdated Show resolved Hide resolved
ixmp/reporting/computations.py Outdated Show resolved Hide resolved
ixmp/reporting/utils.py Show resolved Hide resolved
tests/test_reporting.py Show resolved Hide resolved
@gidden
Copy link
Member

gidden commented May 31, 2019

Hey @khaeru would you mind rebasing this on latest master? I think that will fix the test failure and let me get started poking around. Thanks!

@gidden
Copy link
Member

gidden commented May 31, 2019

yay!

@khaeru khaeru added this to the 0.2 milestone Jun 3, 2019
@khaeru
Copy link
Member Author

khaeru commented Jun 3, 2019

Per discussion today with @volker-krey and @gidden, the plan is now:

  1. Merge this into master more or less as-is,
  2. Release it with 0.2 in the next week or two as clearly-marked ‘experimental’ code,
  3. Refine with smaller PRs based on dogfooding with 2019 YSSPers and their supervisors, and
  4. Promote from ‘experimental’ status in a major release (1.0, probably late 2019).

@khaeru khaeru requested a review from gidden June 4, 2019 08:54
@khaeru
Copy link
Member Author

khaeru commented Jun 4, 2019

  • A3. Support basic operations:
    • A3iii. Interpolation: linear, exponential, etc.
  • A7. Support renaming of quantities using multiple name mappings:
    • A7ii. …loaded from file.
  • A11. Callable through retixmp (tests require Test rixmp using 'testthat' #135)

These will be addressed in separate PRs.

  • Decide: this adds dependencies on dask[array] (thus toolz), graphviz (for dask.visualize), pint, and xarray. Should these be non-optional requirements, or extras (ixmp[reporting])?

@gidden would appreciate your input on this as part of your review. Currently they are added as mandatory requirements of the package.

@gidden
Copy link
Member

gidden commented Jun 6, 2019

Regarding your last point and prior experience, I think we want these to be mandatory deps

@gidden
Copy link
Member

gidden commented Jun 6, 2019

I will start adding notes here (and iiasa/message_ix#176 where appropriate) as I continue to fiddle with things.

TODO:

The current error provided when units are unknown to pint is not useful - it goes into dask internals and errors because a NoneType was provided. The entry point to the error is:

---------------------------------------------------------------------------

AttributeError                            Traceback (most recent call last)

<ipython-input-36-51e83ee23d33> in <module>

----> 1 rep.get(rep.full_key('out'))



~/.local/lib/python3.7/site-packages/ixmp-0.1.3.post0.dev99-py3.7.egg/ixmp/reporting/__init__.py in get(self, key)

   248         log.debug('Cull {} → {} keys'.format(len(self.graph), len(dsk)))

   249

--> 250         return dask_get(dsk, key)

We should catch these prior to calling dask_get() and return a more useful error message

@khaeru khaeru changed the title WIP: Improve reporting (phase 2) Improve reporting (phase 2) Jun 13, 2019
@khaeru khaeru mentioned this pull request Jun 13, 2019
27 tasks
@khaeru
Copy link
Member Author

khaeru commented Jun 13, 2019

Superseded by #144.

@khaeru khaeru closed this Jun 13, 2019
@khaeru khaeru deleted the feature/reporting2 branch June 13, 2019 19:45
@gidden gidden mentioned this pull request Jun 19, 2019
29 tasks
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.

3 participants