-
Notifications
You must be signed in to change notification settings - Fork 14
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
Check if constituents of common regions are in native regions #463
Check if constituents of common regions are in native regions #463
Conversation
The two tests failing are caused by the common-definitions hash pointing to a version where the error found here remains: nomenclature/tests/data/region_processing/external_repo_test/nomenclature.yaml Lines 4 to 7 in 941fc73
Safe to update hash, @danielhuppmann? |
@dc-almeida, yes please go ahead and update the hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two smaller changes would be good to implement.
In addition, technically, the solution implemented does not cover all cases.
Something like this, albeit exotic, should still work:
model: model_a
common_regions:
- region_a
- region_b
exclude_regions:
- region_c
common_regions:
- common_region_1:
- region_a
- region_b
- region_c
Currently, I think it would raise an error because it does not find region_c
in common_regions
.
I've never really seen a use case like that though so, I'd say it's fine for now. What do you think @danielhuppmann?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small suggestion, after that, good to merge (without the need for another review)
Closes #443
Constituent regions used for region aggregation should also be native regions, otherwise, unexpected errors may appear (as caught in this PR.