Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75f727d5b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AND lower(trim(o.unit)) IN ( | ||
| 'm', | ||
| 'meter', | ||
| 'meters', | ||
| 'metre', | ||
| 'metres', | ||
| 'ft', | ||
| 'foot', | ||
| 'feet' |
There was a problem hiding this comment.
ftbgs units when selecting groundwater observations
The new unit whitelist in _create_water_elevation_view() excludes observations whose unit is ftbgs, even though ftbgs is a valid lexicon unit (core/lexicon.json, around line 843). For wells whose latest (or only) groundwater-level observation uses ftbgs, this CTE now drops the record entirely, so ogc_water_elevation_wells can lose rows after this migration. Please add handling for ftbgs (and its depth semantics) instead of filtering it out.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the ogc_water_elevation_wells materialized view to make units explicit and ensure groundwater level observations recorded in meters are normalized to feet for the derived OGC “water elevation” layer.
Changes:
- Update the
ogc_water_elevation_wellsmaterialized view to compute depth-to-water in feet (handling bothmandftunits) and rename output columns with explicit unit suffixes. - Update existing OGC tests to use the renamed unit-explicit columns.
- Add a new test covering normalization of meter-based groundwater level observations to feet.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/test_ogc.py |
Updates assertions for renamed unit-explicit columns and adds a test for meter→feet normalization behavior. |
alembic/versions/n7a8b9c0d1e2_fix_water_elevation_units_to_feet.py |
Alters the view definition to normalize depth-to-water units to feet and renames view columns to include unit suffixes; includes downgrade restoration. |
You can also share your feedback on Copilot code review. Take the survey.
| meter_observation = groundwater_level_observation.__class__( | ||
| observation_datetime=datetime(2025, 1, 2, 0, 4, 0), | ||
| sample_id=groundwater_level_observation.sample_id, |
There was a problem hiding this comment.
observation_datetime is passed a naive datetime(...) while other observation fixtures use UTC timestamps (e.g., ISO strings with Z). For a timestamp with time zone column, naive datetimes are interpreted in the DB/session timezone, which can make the “latest” observation selection in the view depend on environment settings. Use a timezone-aware UTC datetime (e.g., datetime(..., tzinfo=timezone.utc)) or an explicit ...Z timestamp string for consistency.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?