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

Handle 'annotations' as abstraction of 'docs' and 'metadata' #322

Closed
wants to merge 16 commits into from

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Apr 28, 2020

This PR:

  • Modifies the base.Backend API:
    • Add Backend.{set,get,delete}_anno and .get_codes.
    • Remove Backend.{set,get,delete}_meta.
  • Implements the new methods in JDBCBackend.
    • Deletes new methods JDBCBackend.{set,get}_doc that were in this class but not added to the base.Backend API.
  • Updates Scenario.{get,set,delete}_meta.
  • Adds Platform methods:
    • annotate, get_annotations, delete_annotations —for a single targets and 1+ annotations.
    • {set,get}_doc —convenience methods for setting the 'doc' annotation of multiple targets.

This approach:

  • Achieves a superset of the functionality targeted by Bulk saving for metadata and exposing documentation API #314 and the same UX.
  • Keeps the Backend API and data model smaller and simpler:
    • +0 methods instead of +2.
    • Uses common semantics for annotations (even though these are presented to the user with a variety of names), instead of introducing slightly different semantics for 'metadata' and 'documentation'.
  • Takes us closer to, rather than further from:,
    • Harmonize handling of codelists #274—by adding .get_codes() and backend.CodeList to make explicit the commonality between different lists of 'codes' (string IDs).
    • SDMX compatibility—by making clear that 'metadata' and 'documentation' are particular applications of annotations.

Some things that could be improved, now or after this is merged into the branch for #314:

  • JDBCBackend.get_codes can't get a list of existing 'metadata categories' that might be annotated.
  • JDBCBackend.get_codes has to use several lines to get a list of run IDs, since I couldn't find a public method in the Java code to return this.
  • CodeList.variable corresponds to the 'timeseries' member of the Java DocumentationDomain enumeration, so jdbc._domain_enum() has to remap this. I think the latter is a misnomer, and uses the phrase 'time series' in a place when it's already used twice (TimeSeries class, and 'time series data' stored in a TimeSeries object). These 'doc' annotations are actually attached to (fragments of) values occurring in the 'variable' dimension of 'time series data', hence the chosen name.

@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #322 into feature/metadata-bulk-update will decrease coverage by 0.21%.
The diff coverage is 91.86%.

Impacted file tree graph

@@                       Coverage Diff                        @@
##           feature/metadata-bulk-update     #322      +/-   ##
================================================================
- Coverage                         97.36%   97.14%   -0.22%     
================================================================
  Files                                41       41              
  Lines                              4322     4376      +54     
================================================================
+ Hits                               4208     4251      +43     
- Misses                              114      125      +11     
Impacted Files Coverage Δ
ixmp/tests/core/test_scenario.py 100.00% <ø> (ø)
ixmp/core.py 94.55% <78.78%> (-1.30%) ⬇️
ixmp/backend/jdbc.py 93.58% <94.64%> (-0.68%) ⬇️
ixmp/__init__.py 100.00% <100.00%> (ø)
ixmp/backend/__init__.py 100.00% <100.00%> (ø)
ixmp/backend/base.py 98.70% <100.00%> (+0.03%) ⬆️
ixmp/tests/backend/test_base.py 98.36% <100.00%> (+0.02%) ⬆️
ixmp/tests/backend/test_jdbc.py 100.00% <100.00%> (ø)
ixmp/backend/io.py 98.55% <0.00%> (-0.02%) ⬇️

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 cdc43f6...2d5af7a. Read the comment docs.

class CodeList(Enum):
"""Lists of codes in the ixmp data model."""
#: Annotation IDs. See :meth:`.set_anno`
metadata = auto()
Copy link
Member

Choose a reason for hiding this comment

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

please change to meta as described in the data model

jdata.put(str(k), v)
self.jindex[s].setMeta(jdata)
return
# 'metadata'
Copy link
Member

Choose a reason for hiding this comment

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

please change to meta

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Clarifying question, copied from the description:

Achieves a superset of the functionality targeted by #314 and the same UX.

I don't agree that this has the same UX - docs and meta are very distinct concepts, stored differently in our current database implementation, and are used differently in the Scenario Explorer. So I think that having distinct functions for {get, set, delete}_meta and {get, set, delete}_docs is a feature, not a problem.

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

Successfully merging this pull request may close these issues.

3 participants