Skip to content

Update ec network normalize to support variable subsitutions#182

Open
Nospamas wants to merge 5 commits intomasterfrom
ec-network-variable-sub
Open

Update ec network normalize to support variable subsitutions#182
Nospamas wants to merge 5 commits intomasterfrom
ec-network-variable-sub

Conversation

@Nospamas
Copy link
Contributor

Updated crmprtd based on bc_hydro network variable substitutions across to the ec network as well.

Current setup does the following variables:

'air_temperature_yesterday_high': 'air_temperature'
'air_temperature_yesterday_low': 'air_temperature'
'total_precipitation': 'total_precipitation'
'wind_direction': 'wind_from_direction'
'wind_gust_speed': 'wind_speed'

Some unknowns for me due to unfamiliarity with the project leaves me with the following questions:

  • Will the duplicated air_temperature be an issue? Will cell method be correctly pulled?
  • A number of the variables coming back from EC in the XML sheet should maybe be 0 not empty, specifically the precipitation related variables. This could do with a sanity check I think
  • I've included total_precipitation in substitutions for completeness but it doesn't switch it to anything
  • There remains a bug? with one of the tests (test_download_cache) related to timezones, it fails on my machine due to not being in UTC I think.

Copy link
Contributor

@jameshiebert jameshiebert left a comment

Choose a reason for hiding this comment

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

Looks great overall, but just a (hopefully) small change to request.


def normalize(file_stream):
return swob_ml_normalize(

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always skeptical when a patch adds LOCs to our repo. :) The approach you've taken here is reasonable, but I don't think that this is (or will be) specific to ECCC. Could you take what you've done here and incorporate it into the swob_ml module? Then if other networks change their variable names in the future, we will have an easy place to add variable mappings.

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.

2 participants