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

Check if constituents of common regions are in native regions #463

Conversation

dc-almeida
Copy link
Collaborator

@dc-almeida dc-almeida commented Jan 28, 2025

Closes #443
Constituent regions used for region aggregation should also be native regions, otherwise, unexpected errors may appear (as caught in this PR.

@dc-almeida
Copy link
Collaborator Author

dc-almeida commented Jan 28, 2025

The two tests failing are caused by the common-definitions hash pointing to a version where the error found here remains:

repositories:
common-definitions:
url: https://github.com/IAMconsortium/common-definitions.git/
hash: cb85704

Safe to update hash, @danielhuppmann?

@dc-almeida dc-almeida self-assigned this Jan 28, 2025
@dc-almeida dc-almeida added the bug Something isn't working label Jan 28, 2025
@phackstock
Copy link
Contributor

@dc-almeida, yes please go ahead and update the hash.

@dc-almeida dc-almeida marked this pull request as ready for review January 29, 2025 10:03
Copy link
Contributor

@phackstock phackstock left a 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?

@dc-almeida dc-almeida requested a review from phackstock January 29, 2025 14:49
Copy link
Contributor

@phackstock phackstock left a 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)

@dc-almeida dc-almeida merged commit fc26782 into IAMconsortium:main Jan 31, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check native regions belong to the same model in mappings
3 participants