Closed danielhuppmann closed 1 year ago
Merging #715 (ff97338) into main (df1df1c) will increase coverage by
0.0%
. The diff coverage is100.0%
.:exclamation: Current head ff97338 differs from pull request most recent head c1e01b9. Consider uploading reports for the commit c1e01b9 to get more accurate results
@@ Coverage Diff @@
## main #715 +/- ##
=====================================
Coverage 94.9% 94.9%
=====================================
Files 58 58
Lines 5914 5952 +38
=====================================
+ Hits 5614 5654 +40
+ Misses 300 298 -2
Impacted Files | Coverage Δ | |
---|---|---|
pyam/core.py | 95.2% <100.0%> (+0.3%) |
:arrow_up: |
tests/test_feature_validation.py | 100.0% <100.0%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
I think the return values should contain all identifiers to missing data points, not just model and scenario as it could otherwise lead to ambiguities.
As an example these data:
Model | Scenario | Region | Variable | Unit | 2020 | 2025 |
---|---|---|---|---|---|---|
model_a | scenario_a | region_a | variable_a | unit_a | 1 | |
model_a | scenario_a | region_b | variable_a | unit_a | 1 | 2 |
with a required data specification:
region: [region_a, region_b]
variable: variable_a
year: [2020, 2025]
would currently return _modela and _scenarioa. That does not tell you the whole story though as for the first line in the data a value for 2025 is missing while for the second line, which is also _modela and _scenarioa, everything is in order.
I think the return values should contain all identifiers to missing data points, not just model and scenario as it could otherwise lead to ambiguities.
It's not quite straightforward to develop a useful interface (or object) for this "complete" return-object.
Riffing off the example above: Should the returned dataframe contain a column "unit"? If yes, the value should probably be None
for all columns... If no, the columns of the returned object depend on the arguments, making automated processing difficult.
Second, I see a problem that computing the missing index will be quite a lot of processing, only to be thrown away later anyway.
Suggestion: we leave as is for now (or even simplify just return bool
) and add an argument "verbose" later?
Suggestion: we leave as is for now (or even simplify just return bool) and add an argument "verbose" later?
My preference would be the latter. Just keeping it as a kind of validator of, "is everything that we require there?". The question of "what is missing?" could almost be a separate one.
I had an idea of a different, maybe more simple, approach that might be worth exploring. It could be an option to simply construct a list of all the combinations of required data input and then loop over them one by one checking if there is data there. During that you would save what's missing. Something like:
def require_data(self, region=None, variable=None, unit=None, year=None, exclude_on_fail=False):
missing_datapoints = {}
# combine "everything with everything"
for datapoint in itertools.product([region, variable, unit, year]):
# iterate over all model, scenario combinations
for model, scenario in self.index:
# obviously this won't work exactly like this but just to illustrate the idea
if datapoint not in self[model, scenario]:
missing_datapoints[(model, scenario)] = datapoint
if missing_datapoints:
return missing_datapoints
I had an idea of a different, maybe more simple, approach that might be worth exploring.
Yes, that's the (only) way to go if we want to have a complete list of all missing (combinations of) required items.
But if the use case (for now) is just to know whether all required items exist and (optionally) exclude those scenarios that do meet the requirement from further processing, checking the length of an existing index is much more efficient than creating a second index and comparing the two indices line-by-line.
Yes, true. I was already thinking towards providing user feedback to point exactly what data points are missing but that’s probably better handled in a different function if we really need that level of precision.
Please confirm that this PR has done the following:
Description of PR
This PR adds a
require_data()
method as a generalized approach torequire_variable()
. The new method returns none if all scenarios have all (combinations) of required values on the dimensions, or it returns a DataFrame of model-scenarios where not all required values are present.The PR also adds a (data) "coordinates" attribute to quickly access the data columns that are not in the index.
Question
The existing validation methods like
require_variable()
are very verbose. Here, I have not added any logging output. Any preferences?