-
Notifications
You must be signed in to change notification settings - Fork 0
Open
Description
Overall this looks good, but there are some minor items, mostly around variables and whitespace. I'm excited to see the final-final globes.
- Better name than
script.ipynbwould be good. - Natural Earth is a another good source for boundaries at a variety of scales and, unlike Esri, wouldn't require attribution for general use.
- There are whitespace issues, particularly around
*and-in your assignments, making your code harder to read in some places, though not much. Overall it's very readable, but do check PEP 8 for whitespace recommendations. You might also try Black to help with this. See the week 6 repository for the example ofnb_blackin notebooks. There are other implementations, as well. - Rather than commenting out a bunch of cells in the notebook that have been run, you might split the notebook into separate notebooks, or add some check to see if the data has already been prepared and tell the user something like, "It looks like these sections have already been run and won't be run now to avoid overwriting data," or something similar.
- You don't have a lot of
urllibuse. If you wind up doing more API calls or fetching in other projects take a look at therequestspackage, it's fairly common. - You have some mixing of case in variable names, especially notable in the
coords_conversiondefinition with theO,R, andSvariables. These should be longer and lowercase (snake_case) or longer and camelCase. Also,Ois a bad choice because it's easily confused with0, depending on the typeface.
Might be cleaner to skip the single-use dict_temp variable and instead set
grid_points = pd.DataFrame.from_dict({"lon": coords_lon, "lat": coords_lat})
Instead of
# create a temporary dictionary for pandas dataframe
coords_lon = []
coords_lat = []
dict_temp = {'lon': coords_lon, 'lat': coords_lat}
# ...
grid_points = pd.DataFrame.from_dict(dict_temp)
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels