sy - adding inferer module

[yisangriB]

No description provided.

[bacetiner]

@yisangriB Thank you for your pull request. I have a couple of comments that might be worth discussing before merging this request. All of my comments are regarding your changes to the AssetInventory, as these changes have the potential to affect how the other BRAILS++ modules behave.

1. In your request, I noticed that you added the convert_footprint_to_centroid argument to the get_geojson, write_to_geojson, and get_dataframe methods. The primary purpose of the first two methods is to generate a GeoJSON representation of assets in an AssetInventory and write them to a GeoJSON file. Given this, adding a convert_footprint_to_centroid argument to these methods seems a bit out of place. It might be more appropriate for this functionality to be in a separate static method, perhaps within brails.utils.GeoTools, that is applied in the process of creating an AssetInventory. The same reasoning applies to the get_dataframe method, but since you were the primary developer of this method, I think it is more appropriate to leave the decision to you on whether to keep the convert_footprint_to_centroid argument with this method in place.

2. I see that your request also introduces a method called update_feature_names, which appears to duplicate the functionality of the existing change_feature_names method. If you believe that the method you suggested adds additional functionality, could you please integrate this new functionality into the existing change_feature_names method instead?

[yisangriB]

Hi @bacetiner, thanks for reviewing the code! Those are great points, and I have incorporated both of your suggestions.

1. A new method convert_polygons_to_centroids() is introduced under AssetInventory class.
2. The duplicated method update_feature_names() is now removed. Just to clarify, note that change_feature_names() was pulled into the main repo only 5 days ago, which is much later than when I pushed the changes.

Thanks again, and also feel free to change the placement as you see it better fit or add more comments

[bacetiner]

@yisangriB Thank you for your edits. Could you please implement convert_polygons_to_centroids() as a static method in the brails.utils.GeoTools class? Based on my understanding, this method converts polygon geometries (rather than inventories) to centroids, and for this reason, it would be more appropriate as a utility function.

[bacetiner]

@yisangriB I also noticed that your pull request includes a file named Berkeley_building.geojson, which is 13.9 MB in size and contains 639,859 lines of data. Our approach so far has been to avoid uploading such large files directly to prevent unnecessary size increases in the repository. Instead, download them on the fly using a static link created on a platform such as Zenodo. Could you please follow this approach for this file?

Alternatively, another option would be to truncate the file to include data for only a few blocks, which should be sufficient to demonstrate the functionality of inference methods.

Happy to discuss this further if you need additional thoughts!