Remove pointless indexsets.tabulate parameter include_data
#138
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partly closes #137: this PR removes the
include_data
parameter, but doesn't add any way to check the length of theindexset.data
. For that you still have toget
theindexset
somehow and access the attribute directly.Please note that I did quickly check if including
len(data)
would easily be possible. I don't think it is becauseindexset.data
is a@property
on the DB model class at the moment, so not loaded automatically when usingtabulate
. That's becausebase.tabulate()
usespd.read_sql()
, which only reads in the columns of the returned data model (not relationships or attributes built on them).In contrast, e.g.
base.list()
returns a list of data models and their attributes can be accessed normally.Thus, it seems to me that there's no quick way to change that for
indexset.tabulate()
, though it can definitely be done; either through rewritingbase.tabulate()
(think:SELECT
all resulting rows first, load their.data
, then turn intopd.DataFrame) or by accessing the DB twice (think: use
base.tabulate()as usual, then use e.g.
base.list()to get the
.datadata and put it into a new column). Neither of those options seems desirable to me at the moment and I don't think it's necessary to comply with the
message_ix` API, so i would ignore that part of #137 for now (at least, if not forever).