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

US_BLS_Jolts data refresh #983

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

swethammkumari
Copy link

IMPORTANT: If any new statistical variable comes in future , it must be added to the dictionaries :

  1. _dcid_map in map_config.py
  2. _mcf_map in mcf_config.py

jolts_df['Value'] = jolts_df['value']

return jolts_df, schema_mapping


def remap_dcid():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add a block comment on what this function is doing.
Since it is only calling print(0 but not returning anything, is this just to print the mcf on console fro debugging or is the output redirected to a file?

Comment on lines 249 to 251
for f_key, f_value in mcf_config._mcf_map.items():
if f_key in line:
if len(line) == len(f_key):
Copy link
Contributor

@ajaits ajaits Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of a loop to check against each entry, can't we lookup the entry by key:

f_value  = mcf_config._mcf_map.get(line)
if f_value:
  line = f_value

@@ -0,0 +1,674 @@
_mcf_map = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the same as map_config.py, can't we just have one of these dicts?

Having two identical dicts makes maintenance long term harder as these two will have to be kept in sync for edits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants