No ob indices anymore#82
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies river-source output generation in DigRivers and refreshes the project’s locked dependency set.
Changes:
- Simplifies
save_river_sources()by removing unused parameters and dropping the open-boundary/namelist file generation logic, leaving JSON output of river source positions. - Cleans up docstrings/type descriptions in
dig_rivers.py. - Updates
poetry.lockwith newer resolved versions and transitive dependencies.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/bathytools/actions/dig_rivers.py | Removes open-boundary file writing, simplifies save_river_sources() API, and adjusts documentation. |
| poetry.lock | Dependency resolution updates (package versions/markers and new transitive deps). |
Comments suppressed due to low confidence (1)
src/bathytools/actions/dig_rivers.py:617
save_river_sourcesno longer generates MITgcm namelist/open-boundary files (the implementation was removed), but the docstring still claims it does. Please update the docstring to describe only the JSON output (or reintroduce the removed behavior elsewhere) so callers/users aren’t misled.
"""
Saves detailed information about river sources into a JSON file and
generates namelists to configure boundary conditions for the MITgcm
model.
This function organizes and structures the river data to specify where
boundary conditions related to rivers need to be applied in the model.
It also produces namelist files that define the open boundaries for
rivers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Direction.EAST: bool(np.any(water_cells[:, -1])), | ||
| } | ||
| LOGGER.debug("Open sides: %s", open_sides) | ||
|
|
There was a problem hiding this comment.
This block still says it is checking whether a side is "open", but the open_sides computation/logging was removed and water_cells is only used later for intersection checks. Please update/remove this comment (or restore the open-side computation if it’s still needed) to avoid confusing future maintainers.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| based on its direction and length. | ||
| A single `Movement`, representing the river's stem based on its | ||
| direction and length. | ||
|
|
There was a problem hiding this comment.
There’s an inconsistent/likely-typo naming between stem_length (config key + docstring) and the parameter/function name steam_length / _steam_length_to_stem. Consider renaming these to stem_length / _stem_length_to_stem (and adjusting related log/error strings) to match the domain terminology and avoid confusion.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
* Initial plan * Fix misleading comment about water_cells usage in dig_rivers.py Co-authored-by: spiani <8956990+spiani@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: spiani <8956990+spiani@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
This PR removes the code inside bathytools that writes the
rivers_open_boundaries.txtfile, since this responsability has been moved to the mitgcm_inputs suite.