Add CountryProcessor#592
Conversation
danielhuppmann
left a comment
There was a problem hiding this comment.
Looks nice, a first round of suggestions and comments below.
|
Is this ready for review yet? |
|
Yes, I'll open it for final review. I left it as a draft because I was going to look into the minimum countries for aggregation question, but meanwhile looked at other tasks so best to close this one and tackle that a bit later. |
danielhuppmann
left a comment
There was a problem hiding this comment.
A few comments inline, and one general question about the architecture - wouldn't it make sense to have a constructor that builds a RegionProcessor class and then applies that class?
Maybe add a RegionProcessor.from_codelist(hierarchy) constructor?
I'd agree with that approach. The current implementation produces quite a lot of code duplication which increases the chance of failure and makes future maintenance harder. |
phackstock
left a comment
There was a problem hiding this comment.
In terms for functionality, I'd say this PR is where we want it to be with the exception of checking a minimum set of countries.
Implementation wise, I want to echo what @danielhuppmann said earlier in the discussion in this PR, CountryProcessor should either be implemented as a subclass of RegionProcessor or generated through a dedicated constructor as part of the RegionProcessor class. The RegionCodeList and model information should be used to generate model mappings on-the-fly that are then applied to incoming data.
|
Applied review suggestions and:
|
danielhuppmann
left a comment
There was a problem hiding this comment.
Thanks, tests and implementation look good, but as detailed below, I would go for the proper subclass-strategy instead of a utility function.
danielhuppmann
left a comment
There was a problem hiding this comment.
Two minor comments, good from my side to merge after a review by @phackstock
Closes #587. Adds a
CountryProcessorto aggregate country-level data into regional levels (R5, R9, R10).Currently implemented with importing common-definitions as a dependency.