Bug fix in valid GIS names used to create water network models #452
Conversation
Minor updates to PR
wntr/network/elements.py
Outdated
There was a problem hiding this comment.
From my understanding, _base_attributes are the attributes which are necessary to add the element to the model, and _optional_attributes can also be added to the element, but are not necessary for the code to run.
If that is the case, we should make sure all possible attributes for each element are included in _optional_attributes. For example, base_demand is not included for junctions, so when we do a GIS roundtrip we lose base_demand information.
There was a problem hiding this comment.
Looking into it more, base_demand is not yet implemented in the read functions. It looks like we always assign a default value. Strangely, we do mention base_demand in the Network IO documentation in the Shapefile section.
We should probably change that example that if we aren't supporting base_demand as a valid column.
There was a problem hiding this comment.
Updated example in kaklise#76
| @@ -470,6 +495,17 @@ def _valid_names(self, complete_list=True, truncate_names=None): | |||
| if truncate_names is not None and truncate_names > 0: | |||
There was a problem hiding this comment.
I think it makes sense to change element_objects to have keys that match the syntax of the corresponding elements. Something like
element_objects = {
'Junction': wntr.network.elements.Junction,
'Tank': wntr.network.elements.Tank,
'Reservoir': wntr.network.elements.Reservoir,
'Pipe': wntr.network.elements.Pipe,
'Pump': wntr.network.elements.Pump,
'Valve': wntr.network.elements.Valve}
There was a problem hiding this comment.
I looked into doing this and it is a bit of a lift since it is used all over the code base and hard to track down. Best to keep it as is...
wntr/gis/network.py
Outdated
| @@ -99,14 +99,18 @@ def _create_gis(self, wn, crs: str = None, pumps_as_points: bool = False, | |||
| Represent valves as points (True) or lines (False), by default False | |||
| """ | |||
|
|
|||
| def _extract_geodataframe(df, crs=None, links_as_points=False): | |||
| # Drop any column with all NaN | |||
| def _extract_geodataframe(df, crs=None, valid_base_names=[], | |||
There was a problem hiding this comment.
Having mutable default arguments valid_base_names=[] can cause unexpected and hard to debug behavior in Python. I try to avoid this wherever I can. Instead we can default to None and check if it is None in the body of the function and then assign it to an empty list.
There was a problem hiding this comment.
Updated default argument in kaklise#76
| @@ -216,7 +216,7 @@ The following example returns valid base GeoJSON column names for junctions. | |||
|
|
|||
| >>> geojson_column_names = wntr.network.io.valid_gis_names() | |||
There was a problem hiding this comment.
I looked back at this portion of the documentation and it is a little confusing how we talk about the valid_gis_names function. It is probably easiest for me to explain by making my suggest updates in a PR to your branch.
There was a problem hiding this comment.
Updated discussion of the function in the user docs in kaklise#76
Summary
The following PR addresses #448 and #449.
node_typeandlink_typeare no longer stored or used in the WaterNetworkGIS GeoDataFrames. The attributes are still included in the dictionary representation of WaterNetworkModels which are used to translate between WaterNetworkModels and the GIS data.Bug fix: Missing commas resulted in errors in the valid GIS names
Bug fix: GeoDataFrames now include valid columns even when all entries in that column are None
Tests and documentation
Acknowledgement
By contributing to this software project, I acknowledge that I have reviewed the software quality assurance guidelines and that my contributions are submitted under the Revised BSD License.