Conversation
brock-hinkson
requested review from
brodiealexander,
danielcumpton,
dylanlee and
quercoak
quercoak
left a comment
quercoak
left a comment
There was a problem hiding this comment.
Looking good! I think the polygon fix is working pretty well. I like the legend and explanation.
A few things to address:
- Buffering CRS should be 5070 for CONUS
- I'm confused why reference flowpaths are used as the ID in the subset map. My assumption is we should be using
fp_id, but if there is reason to keep it with the reference let me know. - I'm confused about whether width and depth are both being used. It seems like only
y_mlgets mapped but both are mentioned in tooltip. - Would you be able to check on the amount of nodatas in
y_ml? The example gage has no datas for half the map which is a little clumsy to show as an example. Maybe we need a note about no data and how it visualizes? If I were using this I would be confused why half the network disappeared in the example gage. - If we are only visualizing the ML data, can you add that to the tooltip?
- I think we should rename 3D to whatever it actually is - channel depth or width. 3D feels ab it generous given it's a 2D map and we are only mapping one dimension at a time (if we are even mapping both).
- Performance: With the addition of polygons, the ras_xs map really fails with the New Mexico bounding box if you try to move the map at all. I think as lines it was okay but too much data as polygons. How do you feel about enforcing an upper limit on xs that can be shown? We could paginate the results for the map and leave the download full. If you have a look at how STAC browsers work, they paginate search results.
| ) | ||
|
|
||
| # Project flowpath line geometry to a metric CRS for buffering (to visualize width as a buffer around the line) | ||
| three_dim_fp_data = reference_flowpaths.to_crs(epsg=32632) |
There was a problem hiding this comment.
Why are you using this CRS? Should be 5070 for conus. This is what you selected: https://epsg.io/32632 which is for
Is there a reason to use reference flowpaths for geom rather than the NHF flowpaths? It makes more sense to me to use the finished product (NHF flowpaths) but if there's a practical reason to use reference let me know
| tooltip=folium.GeoJsonTooltip( | ||
| fields=[fp_popup_fields[0], "width", "depth"], | ||
| aliases=["Reference Flowpath ID:", "Width (ft):", "Depth (ft):"], | ||
| aliases=["Reference Flowpath ID:", "Width (m):", "Depth (m):"], |
There was a problem hiding this comment.
If we change from reference flowpaths change the alias here
| fp_hl_style = STYLE_MAP["flowpaths"].get("highlight_styling", {}) | ||
|
|
||
| # Project flowpath line geometry to a metric CRS for buffering (to visualize width as a buffer around the line) | ||
| fp_data = fp_data.to_crs(epsg=32632) |
| }, | ||
| popup_keep_highlighted=True, | ||
| ) | ||
| m.add_child(folium.FeatureGroup(name="3D Flowpaths", show=False).add_child(three_dim_fp_poly)) |
There was a problem hiding this comment.
Can we call this feature channel width? I think 3D is a bit generous given it's still a 2d map.
| "The subset returned will include only cross-sections that fully fit into the bounding box." | ||
| ) | ||
|
|
||
| three_dim_flowpath_tooltip = """The 3D Flowpaths map layer visualizes flowpath width and depth. |
There was a problem hiding this comment.
It looks like the legend in the subsetter only has depth legend. Are we using width somewhere? I don't think a single layer would accommodate both depth and width unless you are calculating the cross-sectional area. I don't recommend that because of the no datas. I'm wondering if it would be better to use topwdth since I think we have that filled out for more places than using y_ml which seems to have a fair amount of no datas.
2 participants
Refined the 3D channel mapping by changing flowpaths from lines to polygons, as well as other misc QoL improvements.
Additions
Changes