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
Saving additional info zone helper #3388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the script on a small residential area in Singapore Woodlands. The changes worked as intended, and the corresponding columns appeared in the Input Editor. The contents of the columns also made sense; only three of the buildings were missing an address - the first corresponded to a small shelter and the other two to an electrical substation. So all of that makes sense.
The only open question in my opinion is the one regarding the string-to-float conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes look good to be. The only question I have is about the removal of the string-to-float conversion for some of the parameters (lines 75-80): What's the reasoning behind removing that section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
Please see my comments below.
selected_columns = list(set(list_of_columns).intersection(set(OSM_COLUMNS))) | ||
shapefile[selected_columns] = shapefile[selected_columns] \ | ||
.fillna(1).apply(lambda x: pd.to_numeric(x, errors='coerce')) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatNif
Correct me if I am wrong. The original Line 75-80 and Line 81-85 are duplicated. Hence, I removed the Line75-80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think you're right. I missed that.
In that case, I think this PR is good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this PR can be merged. The code snippet converting the parameters has been removed since it was duplicated right after.
In addition to the internal CEA handle of
Building Name
, such as B1000, and B1001, this PR updates zone_helper to save additional information, includinghouse number
,street name
,house_name
,postal code
, city,residential_type
(to indicate Singapore's HDB),city
, andcountry
.