Script to convert UN codelist to pvmap#2092
Conversation
- Extract modular helpers (normalize_skip_concepts, resolve_output_file) - Add full Google Python style guide docstrings and 80-column compliance - Add comprehensive 8-test unit verification suite
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces scripts, templates, and schemas to automate UN codelist mappings and generate statistical variable (statvar) groups, names, and DCIDs, alongside integrating a custom statvar DCID generator into the importer. The review feedback highlights a few high-to-medium severity issues: a typo and malformed regex pattern in clean_value_str, a logically inverted condition in statvar_dcid_gen.py that could lead to an AttributeError, a duplicate dictionary key in config_flags.py, and an unused variable add_prop in statvar_dcid_gen.py.
…andard separators
| _SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) | ||
| sys.path.append(_SCRIPT_DIR) | ||
| sys.path.append(os.path.dirname(_SCRIPT_DIR)) | ||
| sys.path.append(os.path.dirname(os.path.dirname(_SCRIPT_DIR))) | ||
| _DATA_DIR = os.path.dirname(os.path.dirname(os.path.dirname(_SCRIPT_DIR))) | ||
| sys.path.append(_DATA_DIR) | ||
| sys.path.append(os.path.join(_DATA_DIR, 'util')) | ||
| sys.path.append(os.path.join(_DATA_DIR, 'tools', 'statvar_importer')) |
There was a problem hiding this comment.
This doesn't smell right. If we have an anticipated file structure, should we take the root as an argument rather than trying to navigate around relative paths?
Ok to punt for a future revision, I could imagine this being prone to access issues when in a containerized runtime
There was a problem hiding this comment.
We use this pattern in the data repo so that py scripts can be invoked from current folder or any other folder with relative paths.
I agree, it is seems like a hack and open to suggestion to preserve the flexibility to invoke a py script from any folder.
| if abs(number) > max_int: | ||
| # Convert very large ints to float with potential loss of precision |
There was a problem hiding this comment.
Have we discussed with Luis implicitly converting some numbers to floats / doubles? I know we called out some instances where it was seen but I don't recall him saying it was ok to lower the precision.
I'm worried about this being a side effect and don't think we can unknowingly convert some numbers on behalf of a data custodian without their knowledge + consent.
There was a problem hiding this comment.
This is required until the rest of the ingestion tools can handle large ints as it blocks the whole import now.
Yes this is a pending open item to fix all through the pipeline.
There was a problem hiding this comment.
max_int is a configurable value for python scripts. Currently set to sys.maxsize by default until downstream tools can support numbers as strings, after which it can be set much higher.
| _SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) | ||
| sys.path.append(_SCRIPT_DIR) | ||
| sys.path.append(os.path.dirname(_SCRIPT_DIR)) | ||
| sys.path.append(os.path.dirname(os.path.dirname(_SCRIPT_DIR))) | ||
| _DATA_DIR = os.path.dirname(os.path.dirname(os.path.dirname(_SCRIPT_DIR))) | ||
| sys.path.append(_DATA_DIR) | ||
| sys.path.append(os.path.join(_DATA_DIR, 'util')) | ||
| sys.path.append(os.path.join(_DATA_DIR, 'tools', 'statvar_importer')) | ||
|
|
There was a problem hiding this comment.
Same as previous comment. This doesn't feel right for path traversal
| """Returns a property for the concept.""" | ||
| c = eval_functions.str_to_camel_case(concept.lower().replace('_', ' ')) |
There was a problem hiding this comment.
Can you remind me of docs we have to clarify the intended name shapes?
This feels fragile as str_to_camel_case allows for the persistence of underscores. But here we strip them to spaces; which will then be preserved as spaces. But then we have code samples with again strip the underscores.
There was a problem hiding this comment.
https://schema.org/docs/styleguide.html says property names should be lowerCamelCase.
The conversion to space is to capitalize the following letter. Both space and _ are eventually removed.
|
|
||
| def to_dcid(code: str) -> str: | ||
| """Replace any non alphanumeric characters with '_'""" | ||
| value = re.sub(r'[^A-Za-z0-9\.]+', '_', code) |
There was a problem hiding this comment.
nitpick: if we're going to replace unmatched characters with '_', should we allow the underscore within the match to avoid needless character swaps?
There was a problem hiding this comment.
this is to replace a sequence of __ with a single underscore.
2 participants
Script to convert UN codelists into PV maps. The pvmap is used to: