Some catchments/divides have incorrectly formatted divide_id

[robertbartel]

The divides layer of the current nextgen_02.gpkg file (MD5: a4cd50cd666f4bb177e7671f253a3393) has a row with divide_id of cat-7e+05 (index 33534). No other rows in this file appear to use this irregular format, though I haven't checked all the other hydrofabric files.

[robertbartel]

Also found cat-1e+06 in nextgen_05.gpkg.

[hellkite500]

@mikejohnson51 should these id formats be considered invalid?

[mikejohnson51]

No I dont see any reason they should be invalid as long as we are using strings. I'd like to clean them up for consistency in readability but a unique string is a unique string

[PhilMiller]

I'd like to make the case for limiting the set of characters used in identifiers, specifically to exclude punctuation that has meaning in shells or regular expressions. Yes, everything downstream could be coded to handle arbitrary characters in the strings, but I think it's less work for the ecosystem at large if the hydrofabric comes with a [edit:] reasonably limited specification of what characters it will actually use.

[mikejohnson51]

Sure 100% agree! That said we haven't given identifiers the full care they need (e.g. should they actually be integers?). I think having an evolving - documented - best practice is great but saying things are invalid is potentially extreme given the hydrofabric can (will?) serve many applications outside of NextGen. In the case of these, they were an odd by product of R concatenation - getting ride of them is easy. I don't want this tiny issue to turn into a major thing at this point though. Larger concern would be if this ID made NextGen fail, why did it?

[PhilMiller]

This particular ID caused ngen to fail because it contained a + character, and the ID was concatenated into a string that was then used as a regular expression pattern to match filenames for forcing data.

The overall NWM project is going to have a huge range of tools and languages consuming names from the hydrofabric. Those IDs are being used in a broad range of ways, including in file names, as database keys, and so forth. Given that, I'd expect that any character appearing in an ID that has special meaning to any bit of code it's passing through will lead to a bug.

Given that we do seem to want to encode kinda-arbitrary information beyond a number into IDs for catchments, nexuses, waterbodies, etc, allowing for strings in general seems reasonable enough.

My proposal for the character set to allow is A-Za-z0-9_- (latin alphabet in upper and lower case, digits, dash, and hyphen). None of those have special meanings in regex, shell syntax, shell globs, or SQL to my knowledge.

[mikejohnson51]

Works for me! I'll add it to our docs next week and get a new HF run made with this and AHPS gages included

[PhilMiller]

This is unlikely to come up, but I realize it may be worth specifying that the first character is not - (and maybe not _ either) to avoid command-line utilities misparsing filenames starting with an ID as flag arguments.

I'll see if someone who knows Javascript/JSON can say whether there are other more-specific patterns that may cause it to parse things in ways it shouldn't

[PhilMiller]

Other input from the framework team was that it would additionally be good if we could assure that the first character of these IDs were a letter, other than x, to additionally guard against mis-parsing as numbers if consumer code tries to be stupid.

• The initial x apparently has been seen interpreted like 0x, meaning a number in hexadecimal.

[mikejohnson51]

In the latest run we get:

net = read_parquet('/Volumes/MyBook/conus-hydrofabric/v22/conus_net.parquet')
sum(grepl("[+]", net$id))
#> [1] 0
sum(grepl("[+]", net$divide_id))
#> [1] 0
sum(grepl("[+]", net$toid))
#> [1] 0

^{Created on 2023-11-28 by the reprex package (v2.0.1)}

Therefore I think this is solved! Will add the context to the Rmds before AGU.