Weather Forecast Retrieval - Update to v0.7 #206
Conversation
Adapt the changes introduced in weather_forecast_retrieval v0.7.
| @@ -1,6 +1,6 @@ | |||
| import numpy as np | |||
| import pandas as pd | |||
| from weather_forecast_retrieval.hrrr import HRRR | |||
| import weather_forecast_retrieval.data.hrrr as hrrr_data | |||
There was a problem hiding this comment.
Why rename it? Could you not just import FileLoader or just do hrrr.FileLoader?
There was a problem hiding this comment.
If you look at the call in line 58 and the import with reanming, this clearly indicates where the FileLoader is coming from. It basically combines the hrrr.data as hrrr_data. Using only hrrr is a very general name, has higher potential to be accidentally overwritten by a local variable, and using hrrr_data then also shows why and what this import does. Not importing the FileLoader also indicates in the lower line that this class is not part of SMRF and rather an imported class from WFR.
There was a problem hiding this comment.
It follows the principle found for instance in the Google Python style guide, with the addition of renaming to prevent the above mentioned potential of error
https://google.github.io/styleguide/pyguide.html#224-decision
There was a problem hiding this comment.
I understand why to rename it, I'm just not a fan as it seems to add an extra layer to the import. To me, this is then an bad name in WFR and should have been named something else to avoid someone using a duplicate variable name.
There was a problem hiding this comment.
I do not agree with calling this a bad name choice in WFR. Naming conventions should not only be driven on how other libraries would use it and also reflect the structure of the library itself. From that regard, the naming was a natural hierarchy to me.
Happy to remove this alias and use only hrrr. Then it is identical to the import as it was before. The alias was a conservative strategy based of observations in the current code base where variable names did not have a consistent descriptive choice.
There was a problem hiding this comment.
I don't think it was a bad name choice either, it is the natural choice.
I think I'm more of a purist in this regard of not renaming packages, it's just a personal preference. I do see the benefit when they have a common name to rename the module. As SMRF/AWSM get to be more verbose, then this shouldn't be an issue.
There was a problem hiding this comment.
Thought about some possible options to establish a common style used across the different libraries.
What do you think of this kind of import style:
import weather_forecast_retrieval as wfrThen the call becomes:
wfr.data.hrrr.FileLoaderThis convention leans on the numpy or matplotlib style, where there is a common acronym for both.
There was a problem hiding this comment.
I like that, especially for WRF, it's such a long repo name.
There was a problem hiding this comment.
Will update with a next PR
|
Travis CI is off and forced the merge. This will be tested in the next PR where the Github actions are being setup. |
Adapt the changes introduced in weather_forecast_retrieval v0.7.