feat(well-inventory): allow wells outside New Mexico (BDMS-1109) - #822
Conversation
validate_model upper-cased utm_zone into a local variable but never wrote the result back to self.utm_zone. A row with "13n" therefore passed schema validation, then failed case-sensitively downstream in services/well_inventory_csv.py with a confusing "Unsupported UTM zone: 13n" error at persist time instead of at validation time. Add a field_validator that strips and upper-cases utm_zone once, so every downstream consumer sees the same canonical value the schema already validated.
The importer only recognized 12N and 13N, NM's two UTM zones, via a fixed dict with a TODO noting more zones would eventually be needed. NMBGMR now needs to import wells from neighboring states, which use UTM zones outside that pair. Replace the fixed dict with a parser that accepts any northern-hemisphere zone from 10N to 19N (the continental US) and derive the EPSG code arithmetically as SRID_NAD83_UTM_BASE + zone, since NAD83 covers the whole range on one consistent datum. This only widens what the domain layer can project; the importer's own zone and coordinate restrictions still apply until the next commit removes them.
The importer rejected any row whose UTM zone or resulting lat/lon fell outside New Mexico, even though nothing downstream (geometry storage, county/state lookup) actually depends on the well being in NM. This blocked importing wells from neighboring states. Replace the NM-specific zone allowlist and bounding box with the domain layer's CONUS-wide zone parser and a generic coordinate sanity range. The remaining check is a plausibility guard against transposed easting/northing or feet-vs-meters entry mistakes, not a border check. Worldwide support was considered and deferred: UTM is not a single global grid, so supporting every zone would mean picking a datum per hemisphere rather than reusing NAD83, and there's no current need for it outside North America.
GeoJSONUTMCoordinates hardcoded utm_zone = "13N" and populate_fields always reprojected the stored WGS84 point to EPSG 26913, regardless of where the point actually is. Any well outside zone 13N read back with a confidently wrong easting/northing labeled "13N": this was already latent for existing 12N wells in western NM, and became a live concern once wells from neighboring states could be imported. Add domain/geospatial.py, computing the UTM zone a longitude actually falls in, clamped to the same 10N-19N range domain/wells.py supports on import. Clamping matters because EPSG only defines "NAD83 / UTM zone nN" for n = 1..23; past that, 269xx codes name unrelated NAD83 state-plane systems, so an uncapped zone number could resolve to a valid but wrong CRS instead of failing loudly. schemas/location.py now derives the SRID and the utm_zone label from the point itself instead of a fixed default.
There was a problem hiding this comment.
Amended after clarifying scope with Jake — the original version of this review argued for expanding UTM support worldwide across the board. That was the wrong frame. The actual constraint is a split:
| layer | scope | code |
|---|---|---|
| Location storage + GeoJSON read path | anywhere on earth | schemas/location.py, domain/geospatial.py |
| AMP water-well CSV ingestion | CONUS | schemas/well_inventory.py, domain/wells.py |
That split resolves most of what I originally flagged, and sharpens the rest. Retracting and strengthening below; the two bug fixes in this PR — normalizing utm_zone casing, and unhardcoding 13N in the GeoJSON response — are still clearly right.
What I'm retracting
These were flagged as limitations. Under the split they're correct as written, and should stay:
domain/wells.py's 10N–19N range. This is the AMP ingestion path. CONUS is the intended scope. Keep it.- The
N-only zone regex. No southern-hemisphere zone should ever come through the well-inventory importer. Keep it. northern = Trueinvalidate_model. Permanently correct on this path, not a latent assumption.- NAD83 269xx on the import side. Right datum for the right region. The 10N–19N range sits comfortably inside EPSG's 1–23 NAD83 UTM series, so the arithmetic is safe here.
COORD_LAT_MIN/MAX,COORD_LON_MIN/MAX. A CONUS-shaped gate on AMP ingestion is exactly what's wanted. See the inline comment for the one change I'd still make (naming and placement, not values).
What this sharpens
The clamp in domain/geospatial.py is now unambiguously a defect rather than a scope decision. That module is on the worldwide side — it serves the GeoJSON read path for every stored Location. Clamping a worldwide input to a CONUS range means a Berlin point reads back as E=4961478.4 N=9352242.0 "19N" NAD83, HTTP 200, no log line. Details in the inline comment.
The real bug generator is that both layers share constants. UTM_ZONE_MIN/UTM_ZONE_MAX live in core/constants.py and are imported by both domain/wells.py (correctly, as an ingestion policy) and domain/geospatial.py (incorrectly, as a projection limit). Two modules sitting adjacent in domain/, both doing UTM zone work, with opposite scopes and a shared constant between them — that coupling is precisely how a CONUS bound ended up applied to the worldwide read path. Worth splitting the constants so the boundary is visible:
# core/constants.py
# AMP water-well ingestion policy: submissions are limited to the
# continental US. Not a projection limit -- see domain/geospatial.py,
# which serves points anywhere on earth.
AMP_UTM_ZONE_MIN = 10
AMP_UTM_ZONE_MAX = 19
AMP_COORD_LAT_MIN, AMP_COORD_LAT_MAX = 18.0, 72.0
AMP_COORD_LON_MIN, AMP_COORD_LON_MAX = -180.0, -66.0Then domain/geospatial.py imports none of them, and the next person can't reach for the wrong bound by accident.
What the worldwide read path needs
domain/geospatial.py has to handle any point we can store. Concretely:
Use WGS 84 UTM, not NAD83. 32600 + zone north, 32700 + zone south, zones 1–60. NAD83 is a North American datum — a NAD83 UTM lookup at Berlin returns zero matching CRS from pyproj, and the 269xx series has real holes: 26924–26928 don't exist, and 26929 is NAD83 / Alabama East (state plane). The WGS 84 series has no holes across 1–60, which means the clamp deletes outright rather than needing a replacement guard.
Datum choice costs nothing numerically — measured the same easting/northing through 26913 vs. the utm library (WGS84 ellipsoid) at 0.17 mm apart, since pyproj applies a ballpark NAD83↔WGS84 transform with no grid shift. NAD83 stays where it belongs, on the AMP import side.
Take the point, not just the longitude. Hemisphere can't be derived from longitude, and UTM is undefined outside 80°S–84°N.
Would pyproj be more robust? On this side, yes — and more than I originally credited. Not for the arithmetic: (lon + 180) // 6 + 1 is exact and matches EPSG, and the Norway/Svalbard exceptions don't apply (checked — pyproj returns 32631 for lon 5° / lat 60°, same as plain arithmetic, because the 32V/31X widening is an MGRS grid-zone convention, not an EPSG CRS definition). What it buys is:
- The EPSG code is looked up, never invented — no possibility of landing on a real-but-unrelated CRS.
- Out-of-domain input fails loudly for free.
query_utm_crs_info(datum_name="WGS 84", area_of_interest=AreaOfInterest(0, 85, 0, 85))returns[]. On a worldwide read path that receives arbitrary stored points, that's the difference between a clear error and a plausible wrong number. - Hemisphere derived from the point.
(-58.4, -34.6)→32721;(151.2, -33.9)→32756.
~33 ms per uncached query (measured 200 at 6.5 s), so memoize on (zone, northern) — not raw lon/lat, or every point misses.
UTM_LAT_MIN, UTM_LAT_MAX = -80.0, 84.0
class OutsideUtmDomain(ValueError):
"""Point falls outside the latitude band UTM is defined for."""
def utm_zone_for_longitude(longitude: float) -> int:
"""Zone number 1-60, with longitude normalized onto [-180, 180)."""
return int(((longitude + 180) % 360) // 6) + 1
@lru_cache(maxsize=128)
def _utm_epsg(zone: int, northern: bool) -> int:
# Resolve against the EPSG database rather than computing a code, so an
# undefined zone raises instead of landing on an unrelated CRS.
lon = (zone - 1) * 6 - 177 # zone central meridian
lat = 1.0 if northern else -1.0
suffix = f"{zone}{'N' if northern else 'S'}"
for info in query_utm_crs_info(
datum_name="WGS 84",
area_of_interest=AreaOfInterest(lon, lat, lon, lat),
):
if info.name.endswith(suffix):
return int(info.code)
raise OutsideUtmDomain(f"No WGS 84 UTM CRS for zone {suffix}")
def utm_crs_for_point(longitude: float, latitude: float) -> tuple[int, str]:
"""Return the (EPSG code, zone label) of the UTM zone containing a point."""
if not UTM_LAT_MIN <= latitude <= UTM_LAT_MAX:
raise OutsideUtmDomain(
f"Latitude {latitude} is outside the UTM domain "
f"({UTM_LAT_MIN} to {UTM_LAT_MAX}); polar points need UPS."
)
zone = utm_zone_for_longitude(longitude)
northern = latitude >= 0
return _utm_epsg(zone, northern), f"{zone}{'N' if northern else 'S'}"Note the zone math also needs % 360 independent of everything else — int((longitude + 180) // 6) + 1 returns 61 at longitude == 180.0, and 62 / −2 for unnormalized input like 185 / -190. The clamp masks all of it today, so removing the clamp surfaces a crash without this.
Antarctica
We have no Antarctic data yet but expect to store Antarctic samples. Under the split this lands squarely on the worldwide read path, and not at all on the AMP importer — those wells are CONUS by policy, so nothing there needs to change.
Two bands:
| band | status | sites |
|---|---|---|
| 60°S – 80°S | UTM valid; southern zones exist (32720, 32758, 32712, 32748) |
Palmer Station, McMurdo, WAIS Divide, Vostok, Dome C |
| below 80°S | UTM undefined. utm.from_latlon raises OutOfRangeError; pyproj returns no UTM CRS |
South Pole, Ross Ice Shelf interior, most of the plateau |
Below 80°S needs UPS South (EPSG 32761) or, more usefully for polar science, Antarctic Polar Stereographic (EPSG 3031). Fine to defer the implementation — but the read path should raise on those points rather than clamp, so the gap is visible the day the data arrives instead of six months later.
Suggested scope for this PR
Small, in scope, and they turn the eventual polar work into a normal ticket rather than a breaking change:
- Replace the clamp with a raise, and rename the shared constants so the CONUS bound can't be reached from the worldwide side.
- Add an explicit
-80.0 / 84.0latitude gate indomain/geospatial.py, with a comment naming EPSG 3031/32761 as the future path. - Make
utm_coordinatesoptional in the GeoJSON response. Turning a required response object optional later, after clients depend on it, is a breaking API change. Doing it now — while every stored point is CONUS and the field is populated on every response anyway — is invisible to every consumer.
The full worldwide WGS 84 UTM switch can be its own ticket. Items 1–3 are what keep this PR from making that ticket harder.
| scope -- see domain/wells.py. | ||
| """ | ||
| zone = int((longitude + 180) // 6) + 1 | ||
| return max(UTM_ZONE_MIN, min(UTM_ZONE_MAX, zone)) |
There was a problem hiding this comment.
Amended after clarifying scope with Jake. Ocotillo should store locations anywhere on earth; AMP water-well ingestion should stay CONUS-only. This module is on the worldwide side, which makes the clamp a defect rather than a scope decision.
The hardcoded 13N was wrong-by-one-zone within New Mexico. This is wrong-by-a-continent, and equally silent. Ran the PR's read path on points outside 10N–19N:
| point | GeoJSON utm_coordinates |
|---|---|
| Berlin (13.4, 52.5) | E=4961478.4 N=9352242.0 "19N" NAD83 |
| Tokyo (139.7, 35.7) | E=-2128591.4 N=15641211.8 "19N" NAD83 |
| McMurdo (166.7, -77.8) | E=-623450.0 N=-10771051.7 "19N" NAD83 |
Negative eastings, northings past the pole distance, HTTP 200, no log line. Every one of those is a point Ocotillo is supposed to be able to store.
The docstring frames clamping as avoiding "a valid but wrong CRS instead of failing loudly" — but the uncapped version doesn't silently succeed either. CRS.from_epsg(26924) raises crs not found (26924–26928 are simply absent; the state-plane collision only starts at 26929). So the choice was never "clamp or silent garbage" — it was "clamp, which is silent garbage" versus "raise, which is loud."
Two ways out, either fine:
- Raise an
OutsideUtmDomainhere for anything outside the supported range. Cheapest, and makes the gap visible. - Switch this module to WGS 84 UTM (
32600/32700 + zone), which has no holes across zones 1–60 — the clamp then deletes outright and only a latitude guard is needed.
Worth noting the CONUS bound arrives here via UTM_ZONE_MIN/UTM_ZONE_MAX, imported from core/constants.py — the same constants domain/wells.py uses correctly as an AMP ingestion policy. Sharing them across that boundary is what let a CONUS limit land on the worldwide path.
| from core.constants import SRID_NAD83_UTM_BASE, UTM_ZONE_MAX, UTM_ZONE_MIN | ||
|
|
||
|
|
||
| def utm_zone_for_longitude(longitude: float) -> int: |
There was a problem hiding this comment.
Amended after clarifying scope with Jake. Ocotillo should store locations anywhere on earth; AMP water-well ingestion should stay CONUS-only. Taking longitude only forecloses two things this path now needs, since it has to serve any stored point:
- Hemisphere. N vs S can't be derived from longitude, so this can never label a southern-hemisphere point. (The mirror assumption in
schemas/well_inventory.pyis fine — that path is CONUS by policy. This one isn't.) - Latitude bounds. UTM is undefined outside 80°S–84°N, and nothing in the codebase checks that today.
Suggest (longitude, latitude) — the call site in schemas/location.py already has the full point in hand, so it costs one argument and lets the polar branch slot in later without touching callers.
Separate from all of the above, the zone math has an edge case the clamp is currently hiding: int((longitude + 180) // 6) + 1 returns 61 at longitude == 180.0, and unnormalized input (185, -190) yields 62 and −2. So removing the clamp surfaces a crash unless this changes too. int(((longitude + 180) % 360) // 6) + 1 holds 1–60 across the range I tested (−190 … 185).
|
|
||
| def srid_for_longitude(longitude: float) -> int: | ||
| """Return the NAD83 UTM EPSG code for the zone a longitude falls in.""" | ||
| return SRID_NAD83_UTM_BASE + utm_zone_for_longitude(longitude) |
There was a problem hiding this comment.
Amended after clarifying scope with Jake. Ocotillo should store locations anywhere on earth; AMP water-well ingestion should stay CONUS-only. NAD83 is the wrong datum for this module specifically. It's correct in
domain/wells.py— AMP wells are CONUS, and 10N–19N sits comfortably inside EPSG's 1–23 NAD83 UTM series. But this path serves points anywhere on earth, and a NAD83 UTM lookup outside North America returns zero matching CRS from pyproj.
SRID_NAD83_UTM_BASE + zone also computes a code rather than looking one up, which is the root reason the clamp above has to exist: 26924–26928 don't exist and 26929+ are state plane, so unguarded arithmetic can land on a real-but-unrelated CRS.
WGS 84 UTM (32600 + zone north, 32700 + zone south) has no holes across 1–60, so switching this module removes both problems at once. The datum change is numerically free — measured 0.17 mm difference through 26913 vs. the WGS84-ellipsoid utm library, since pyproj applies a ballpark NAD83↔WGS84 transform with no grid shift.
| # easting/northing and feet-vs-meters entry mistakes. It is wider than the US | ||
| # (e.g. it admits Mexico City) -- do not use it to decide "is this the US". | ||
| COORD_LAT_MIN, COORD_LAT_MAX = 18.0, 72.0 | ||
| COORD_LON_MIN, COORD_LON_MAX = -180.0, -66.0 |
There was a problem hiding this comment.
Amended after clarifying scope with Jake. Ocotillo should store locations anywhere on earth; AMP water-well ingestion should stay CONUS-only. Retracting my earlier comment here — I originally flagged this range as blocking worldwide support. It isn't. This gates AMP water-well ingestion, which is CONUS by policy, so a CONUS-shaped bound is exactly right. Values stay.
One change I'd still suggest, though: name and place them so they read as ingestion policy rather than as a general-purpose coordinate limit.
# AMP water-well ingestion policy: submissions are limited to the
# continental US. Not a projection limit -- see domain/geospatial.py,
# which serves points anywhere on earth.
AMP_COORD_LAT_MIN, AMP_COORD_LAT_MAX = 18.0, 72.0
AMP_COORD_LON_MIN, AMP_COORD_LON_MAX = -180.0, -66.0Same argument applies more urgently to UTM_ZONE_MIN/UTM_ZONE_MAX two lines up: those are imported by both domain/wells.py (correct — ingestion policy) and domain/geospatial.py (incorrect — applied as a projection limit on the worldwide read path). Generic names on a shared constant are what made that mistake available. An AMP_ prefix, and domain/geospatial.py importing none of them, closes it off.
Minor accuracy note while here: the existing comment calls this "a coarse sanity range, not a national border." Accurate in spirit, but it is CONUS-shaped — -180.0 … -66.0 excludes the entire eastern hemisphere. Saying so plainly makes it obvious why the read path must not borrow it.
| SRID_UTM_ZONE_12N = 26912 | ||
|
|
||
| # EPSG 269xx == NAD83 / UTM zone xxN. Zones 10N-19N span the continental US. | ||
| SRID_NAD83_UTM_BASE = 26900 |
There was a problem hiding this comment.
Small doc accuracy point: the comment says "EPSG 269xx == NAD83 / UTM zone xxN," which holds for 26901–26923 but not past it. 26924–26928 don't exist, and 26929+ are NAD83 state plane (26929 = Alabama East). Worth stating the 1–23 bound here since this constant is what makes the arithmetic possible, and the bound is the reason the range check downstream isn't optional.
| data_dict["properties"]["utm_coordinates"][ | ||
| "northing" | ||
| ] = point_utm_zone_13n_wkt.y | ||
| data_dict["properties"]["utm_coordinates"]["easting"] = point_utm_wkt.x |
There was a problem hiding this comment.
Amended after clarifying scope with Jake. Ocotillo should store locations anywhere on earth; AMP water-well ingestion should stay CONUS-only. This is the right fix for the hardcoded
13N— good catch on it being a quiet bug that stops being quiet under this PR.
One ask while this block is already being touched: make utm_coordinates optional on GeoJSONProperties.
utm_coordinates: GeoJSONUTMCoordinates | None = NoneToday it's Field(default_factory=GeoJSONUTMCoordinates) — always present, never null. Since Ocotillo stores points anywhere on earth, this response has to cope with points that have no UTM coordinates at all: anything below 80°S or above 84°N, where UTM is undefined. The right answer for those is to omit the block, not invent a label. Nothing is lost — the geometry member already carries WGS84 lon/lat.
The reason to do it in this PR rather than later is purely API compatibility: changing a required response object to optional after clients depend on it is breaking. Right now every stored point is CONUS and this field is populated on every response, so the change is invisible to every consumer. That window closes as soon as the first non-CONUS location lands.
Related: GeoJSONUTMCoordinates.horizontal_datum is hardcoded "NAD83" (line 92, outside this diff). Correct for everything stored today, but it becomes wrong for any point outside North America, so it should be derived alongside utm_zone whenever this is revisited.
| (170.0, 19), # far east of CONUS -- clamps rather than picking zone 51 | ||
| ], | ||
| ) | ||
| def test_utm_zone_for_longitude_clamps_outside_conus(longitude, expected_zone): |
There was a problem hiding this comment.
This test pins the clamp as intended behavior, which is what makes the clamp expensive to remove later — the failing test reads as a regression rather than the fix.
If the clamp becomes a raise, this inverts to something like:
@pytest.mark.parametrize("longitude", [-170.0, 170.0])
def test_utm_zone_for_longitude_rejects_outside_conus(longitude):
with pytest.raises(UnsupportedUtmZone):
utm_zone_for_longitude(longitude)Worth adding a 180.0 case either way — the current implementation returns zone 61 for it, which only looks fine because the clamp swallows it.
| "13N": SRID_UTM_ZONE_13N, | ||
| "12N": SRID_UTM_ZONE_12N, | ||
| } | ||
| UTM_ZONE_REGEX = re.compile(r"^\s*(\d{1,2})\s*N\s*$", re.IGNORECASE) |
There was a problem hiding this comment.
Amended after clarifying scope with Jake. Ocotillo should store locations anywhere on earth; AMP water-well ingestion should stay CONUS-only. Retracting the main point I made here. I'd suggested widening this to
[NS]for southern-hemisphere support. That's wrong for this module —domain/wells.pyserves AMP water-well ingestion, which is CONUS by policy, so rejecting anSsuffix outright is correct and should stay. Same for the 10N–19N bound below.
The one thing I'd still keep from the original comment is much smaller: the rejection message. Unsupported UTM zone: 13S gives the submitter no signal about whether the problem is the 13 or the S. Since the range is now a deliberate policy rather than an implementation limit, it's worth saying so — something like "southern-hemisphere zones are not accepted; AMP well submissions are limited to CONUS zones 10N-19N."
\d{1,2} admitting up to 99 is fine given the range check downstream. Not worth changing.
| # outside the supported northern-hemisphere CONUS range; Pydantic wraps it | ||
| # with its own "Value error, " prefix, so no message is composed here. | ||
| zone = utm_zone_number(self.utm_zone) | ||
| northern = True # utm_zone_number rejects anything but an "N" suffix |
There was a problem hiding this comment.
Amended after clarifying scope with Jake. Ocotillo should store locations anywhere on earth; AMP water-well ingestion should stay CONUS-only. Softening this one.
northern = Trueis not a latent assumption on this path — AMP ingestion is CONUS by policy, so it's permanently correct. The comment explaining it is accurate. Nothing to change.
The observation about test_badly_scaled_coordinates_raise_out_of_range_error still holds, though, as a note on what's actually guaranteeing correctness here. It's true that utm.to_latlon rejects an out-of-domain easting, but the library validates northing range and zone number, never latitude:
utm.to_latlon(500000, 500000, 58, northern=False) -> lat -85.540 no error
utm.to_latlon(500000, 0, 58, northern=False) -> lat -90.018 no error
That last return value isn't a latitude. Not reachable through this importer — the CONUS coordinate range gates it well before that — so this is informational rather than a request. Just worth knowing that the bounding box, not the library, is doing the real work, in case the box is ever loosened.
PR review on #822 pointed out that UTM_ZONE_MIN/MAX and COORD_LAT/LON_MIN/MAX read as general-purpose limits, but they are AMP water-well ingestion policy: domain/wells.py uses them correctly to gate CSV imports to CONUS, while domain/geospatial.py (the GeoJSON read path, which must serve any stored point) imported the same names as if they were a projection limit. Two modules doing UTM zone work with opposite scopes sharing one generic-named constant is exactly what let a CONUS bound apply to the worldwide read path. Rename to AMP_UTM_ZONE_MIN/MAX and AMP_COORD_LAT/LON_MIN/MAX, with a comment stating the policy explicitly and pointing at domain/geospatial.py as the module that must not import them. Also corrects the EPSG comment: "269xx == NAD83 / UTM zone xxN" only holds for zones 1-23; 26924-26928 don't exist, and 26929+ names unrelated NAD83 state-plane systems. domain/wells.py's rejection message now names the policy directly ("AMP well submissions are limited to CONUS zones 10N-19N") instead of a bare "unsupported", since the range is a deliberate choice, not an implementation limit. No behavior change. domain/geospatial.py still uses the (renamed) CONUS bound here; the next commit removes that dependency entirely.
Review on #822 caught a real defect in the clamp added by the previous commit: it made the bug worse, not better. A Berlin point reprojected through NAD83 zone 19N and came back as E=4961478.4 N=9352242.0, "19N", NAD83. That's a real coordinate elsewhere on Earth, returned with HTTP 200 and no error. I reproduced this against the actual code before trusting the review's numbers. The root problem was scope, not arithmetic. domain/geospatial.py backs the GeoJSON read path for every stored Location, and the product intends those to sit anywhere on earth. But the module computed a NAD83 EPSG code, valid only for zones 1-23 (a slice of North America), and clamped anything outside that range instead of rejecting it. NAD83 has no southern-hemisphere zones at all, and the old function took longitude only, so hemisphere could never be derived. AMP's water-well ingestion (domain/wells.py) keeps NAD83 and a CONUS-only zone range; that half of the split was already correct and stays as-is. Replace the arithmetic with a CRS lookup against pyproj's EPSG database (query_utm_crs_info), memoized per (zone, hemisphere) since each lookup costs about 33ms uncached. Take (longitude, latitude) instead of longitude alone, so hemisphere comes from the point rather than an assumption. Add a -80/84 latitude gate matching UTM's actual domain, raising OutsideUtmDomain past it instead of guessing; polar regions need UPS/Polar Stereographic, which this doesn't implement. The zone-number formula had its own edge case the clamp was hiding: unnormalized longitude (180.0, 185, -190) returned zone 61, 61, and -1. Normalizing onto [-180, 180) first fixes it. schemas/location.py now derives horizontal_datum from the same lookup (WGS84, not a hardcoded NAD83) and makes utm_coordinates optional, omitting it for points OutsideUtmDomain instead of inventing a label. The geometry member already carries WGS84 lon/lat for those. Making the field optional now, while every stored point is CONUS, avoids a breaking API change later once a client depends on it being present. Antarctic and polar support (EPSG 3031/32761 for below -80) is left for later. Raising there instead of silently mislabeling keeps the gap visible until that lands.
Coverage✅ 79.54% total — gate is 75%. Coverage for the Python files changed in this PR
|
|
@jirhiker I pushed two follow-up commits addressing the review:
The table above also covers all three "suggested scope for this PR" items from the review summary: raise instead of clamp plus rename the shared constants, add the -80°/84° latitude gate with a comment naming the future EPSG path, and make Full pytest suite: 834 passed, 0 failed. Both directly-affected feature files ( Antarctic and polar support (EPSG 3031/32761, below -80°) is intentionally deferred. The read path raises there now instead of silently mislabeling, so the gap stays visible until that's implemented. |
…ocation-restriction-bdms-1109
Coverage✅ 79.36% total — gate is 75%. Coverage for the Python files changed in this PR
|
2 participants
Summary
Drops the New Mexico-only restriction on well inventory CSV imports and expands supported UTM zones from just 12N/13N to the full continental US range (10N-19N, NAD83). Along the way I also fixed two bugs I ran into: a lowercase zone value like "13n" was slipping past schema validation but blowing up later at persist time, and the location GeoJSON endpoint was hardcoding UTM zone "13N" no matter where a well actually was. Review on this PR caught that my first pass at that second fix was itself wrong in a bigger way, so the GeoJSON read path now resolves any point on Earth correctly instead of just NM.
Why
This PR addresses the following problem / context:
schemas/well_inventory.py'svalidate_modelhad two hardcoded, coupled checks: an allowlist of exactly{"12N", "13N"}, and a lat/lon bounding box matching New Mexico's borders. Anything outside either one got rejected at validation. NMBGMR needs to bring in wells from neighboring states now, and this check was really just an input gate. Nothing downstream actually cares whether the well is in NM:transform_sridis just generic pyproj,Location.county/.stateare nullable free text pulled from Census TIGERWeb rather than an NM-specific lookup, and there's noCheckConstrainton the geometry column enforcing this either.domain/wells.pyeven has a# TODO: this needs to be more sophisticated in the future. Likely more than 13N and 12N will be usedsitting in it, which is basically calling out the exact limitation this PR removes.I also found a pre-existing bug in the same function:
validate_modelupper-casesutm_zoneinto a local variable but never actually writes it back toself.utm_zone. So a row with "13n" would pass schema validation fine, then fail case-sensitively down inservices/well_inventory_csv.pyat persist time with a not-very-helpful "Unsupported UTM zone: 13n" error. That's unrelated to the NM restriction, so I split it into its own commit rather than bundling it with the feature change.A second bug turned up during review as well:
schemas/location.py'sGeoJSONUTMCoordinateshardcodesutm_zone = "13N"and always reprojects the stored point to EPSG 26913, regardless of where the point actually is. This was already wrong for 12N wells out in western NM, but it was a quiet bug since it never really surfaced. It stops being quiet the moment this PR starts allowing wells outside 12N/13N. Those wells would import fine and then read back with a confidently wrong easting/northing labeled "13N."My first fix for that, adding a CONUS clamp so any longitude maps to a zone between 10N-19N, turned out to be a worse version of the same bug, and review on this PR caught it: a Berlin point reprojected through NAD83 zone 19N and came back as
E=4961478.4 N=9352242.0, "19N", NAD83. That's a real coordinate elsewhere on Earth, returned with HTTP 200 and no error. The actual scope issue is thatdomain/geospatial.pybacks the GeoJSON read path for every storedLocation, and the product intends those to live anywhere on Earth.domain/wells.py's CONUS-only, NAD83-based zone range is correct as-is for AMP water-well ingestion specifically; it was never meant to bound the read path too. Two follow-up commits fix that (see Changes).Worldwide import support is still out of scope on purpose: UTM isn't one global grid, so accepting every zone on ingestion would mean picking a datum per hemisphere instead of just reusing NAD83, and there's no real ingestion need for that outside North America right now. The read path is a different story, covered below.
Changes
Implementation summary - the following was changed / added / removed:
field_validatorthat strips and upper-casesutm_zoneonce, so every downstream consumer sees the canonical value the schema already validated instead of persisting something different from what it checked.domain/wells.py's fixed{"13N": 26913, "12N": 26912}dict is gone.utm_zone_number()replaces it with a regex parser accepting any northern-hemisphere zone from 10N-19N, andsrid_for_utm_zone()derives the EPSG code asSRID_NAD83_UTM_BASE + zone, since NAD83 covers the whole range on one datum.validate_modelno longer checks against NM's lat/lon box or the old two-zone allowlist. It now callsdomain.wells.utm_zone_numberand checks against a generic coordinate sanity range, a plausibility guard against transposed easting/northing or feet-vs-meters entry mistakes. The comment says explicitly that it's not a border check.UTM_ZONE_MIN/MAXandCOORD_LAT/LON_MIN/MAXare nowAMP_UTM_ZONE_MIN/MAXandAMP_COORD_LAT/LON_MIN/MAX, with a comment stating plainly that this is AMP water-well ingestion policy, not a projection limit. I also fixed a comment that overclaimed EPSG 269xx coverage: it's only valid for zones 1-23, and past that the numbers belong to unrelated NAD83 state-plane systems.domain/wells.py's rejection message now names the policy directly instead of just saying "unsupported."domain/geospatial.pynow resolves the true UTM zone and EPSG code for any point with a pyproj CRS lookup (query_utm_crs_info, memoized per zone/hemisphere), instead of computing a NAD83 code and clamping it. It takes(longitude, latitude)since hemisphere can't be derived from longitude alone, and it raisesOutsideUtmDomainfor the polar latitudes outside -80°/84° that UTM doesn't cover. Support for those is deferred, but now the gap is visible instead of silently wrong.schemas/location.pyderiveshorizontal_datumdynamically ("WGS84", not a hardcoded "NAD83") and makesutm_coordinatesoptional, omitting it for out-of-domain points instead of inventing a label. Doing that now, while every stored point is CONUS, avoids a breaking API change later.Verification
pytest tests/test_domain_wells.py tests/test_well_inventory.py tests/test_cli_commands.py tests/test_domain_geospatial.py tests/test_location.py: 176 passed, includingTestWellInventoryRowUtmValidation(lowercase normalization, CONUS-boundary rejection, generic-range rejection, theutmlibrary's ownOutOfRangeErrorsurfacing as a per-row error rather than an aborted run), the rewrittentest_domain_geospatial.py(zone/CRS resolution for both hemispheres, the -80/84 latitude gate, the zone-formula edge cases at ±180°), andtest_location.pycases proving an 11N well reports correctly, a southern-hemisphere point gets an "S" zone instead of a fake northern one, and a polar point omitsutm_coordinatesinstead of erroring.behave tests/features/well-inventory-csv.feature: 45/45 passing.behave tests/features/well-core-information.feature: same single pre-existing, unrelated error as before this PR (aKeyErrorin an unrelatedGivenstep, confirmed by reproducing it with these changes stashed out).openpyxldependency in the local venv).Notes
Any special considerations, workarounds, or follow-up work to note?
services/util.py'snormalize_datetime_to_utcstill assumes timezone-naive CSV timestamps are Mountain Time. The original justification for that ("location is restricted to New Mexico") is gone now, but the conversion logic is shared with the water-level CSV importer too, so I didn't want to change behavior there as a side effect of this PR. Covered for now with the doc and comment update above; the real fix, deriving timezone from the coordinate or requiring an explicit offset, deserves its own ticket.OutsideUtmDomainthere instead of silently mislabeling keeps the gap visible until that's implemented. Worth flagging since Ocotillo expects to store Antarctic samples soon.