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

added new census distribution over buildings UDF #97

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

milind-soni
Copy link
Contributor

Added New population Distribution UDF

Comment on lines +183 to +184


Copy link
Contributor

Choose a reason for hiding this comment

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

Please run black to autoformat.


#shorten the column name
gdf.columns = gdf.columns.map(lambda x:(str(x.split('|')[0])+str(x.split('|')[-1])) if '|' in x else x)
print(gdf.columns)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for print statement in helper function.

suffix='centroid'
print(suffix)

#read the variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#read the variables
# Read the variables

Please standardize all comments to use sentence case and have a space between the # and first letter, and add this to the contributing md.

return List[0][0]


import geopandas as gpd
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run isort on every new .py file. This import statement should be at top of file.


@fused.cache
def acs_5yr_meta(short=True):
import pandas as pd
Copy link
Contributor

@pgzmnk pgzmnk Jun 10, 2024

Choose a reason for hiding this comment

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

Import statement at top of file. If it were a "rare" import statement that introduced latency at import, it'd make sense to have it within the function. In this case, pandas is common enough so it belongs at the top of the file.

@@ -0,0 +1,35 @@
import geopandas as gpd
Copy link
Contributor

Choose a reason for hiding this comment

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

In UDFs, import statement should go inside the UDF. You already have this import in line 11.

@pgzmnk
Copy link
Contributor

pgzmnk commented Jun 10, 2024

A naiive run of the UDF gives the following error at some zoom levels. What error-handling can be done to print a statement that is helpful to the user? When this error shows up, it's not clear for the user why it happens nor how to resolve it.

image


@fused.cache
def get_census(bbox, census_variable='Total Pop', scale_factor=200, is_density=True, year=2022):
from utils import acs_5yr_bbox
Copy link
Contributor

Choose a reason for hiding this comment

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

This import statement is incorrect. This function is already in the utils file so acs_5yr_bbox is already in scope and needn't be imported.

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.

None yet

2 participants