Got an extra set of eyes looking at the code, and it identified a few bugs. I'll leave it here for now, until I find time to pick up the implementation.
Where: nlmod/gwf/lake.py:210-219, in lake_from_gdf (and docstring at :70-71).
Problem. lake_from_gdf treats any string lakeout value as a lake boundname to resolve. An out-of-model outlet is documented to require the integer -1 (flopy zero-based). If the input GeoDataFrame stores the outlet as the string '-1' (or any string that is not an existing lake boundname), the boundname mask is empty and gdf.loc[mask, "lakeno"].iloc[0] raises an opaque IndexError instead of either creating the out-of-model outlet or raising a clear error.
Reproduction. A GeoDataFrame with lakeout='-1' (string) on a lake, passed to lake_from_gdf. (Encountered with an NHFLO lakes_pwn dataset; the data side is being fixed to use integer -1, but the function could be more robust.)
Suggested fix. Coerce numeric strings before the boundname lookup (so '-1' -> out-of-model outlet), and raise a clear KeyError/ValueError naming the unresolved boundname when a string lakeout matches no lake. Possibly related: #500 (representation of empty fields in lake package construction).
Got an extra set of eyes looking at the code, and it identified a few bugs. I'll leave it here for now, until I find time to pick up the implementation.
Where:
nlmod/gwf/lake.py:210-219, inlake_from_gdf(and docstring at:70-71).Problem.
lake_from_gdftreats any stringlakeoutvalue as a lake boundname to resolve. An out-of-model outlet is documented to require the integer-1(flopy zero-based). If the input GeoDataFrame stores the outlet as the string'-1'(or any string that is not an existing lake boundname), the boundname mask is empty andgdf.loc[mask, "lakeno"].iloc[0]raises an opaqueIndexErrorinstead of either creating the out-of-model outlet or raising a clear error.Reproduction. A GeoDataFrame with
lakeout='-1'(string) on a lake, passed tolake_from_gdf. (Encountered with an NHFLOlakes_pwndataset; the data side is being fixed to use integer-1, but the function could be more robust.)Suggested fix. Coerce numeric strings before the boundname lookup (so
'-1'-> out-of-model outlet), and raise a clearKeyError/ValueErrornaming the unresolved boundname when a stringlakeoutmatches no lake. Possibly related: #500 (representation of empty fields in lake package construction).