Conversation
- Introduced a new optional parameter 'group' to allow grouping of wells. - Updated documentation to reflect changes in well rate handling for grouped wells. - Added validation to ensure unique DataFrame index and consistent flow rates within groups.
|
Looks good! The function is a bit hard to check because the function is pretty lengthy but I think the approach is good. 2 remarks:
|
Yes, it is not running correctly yet. I'll ask you for another review once fixed |
|
Okay, I will check it later. To avoid having your PR reviewed to soon you can also convert the PR to a draft ("Convert to draft" link in the side pane below "reviewers"). Once you are finished you can use the "Ready for Review" button to let me know I can start reviewing. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for grouping wells in the Multi-Aquifer Well (MAW) functionality. Wells can now be grouped together such that they share the same pump and flow rate, which is useful for scenarios where multiple wells are connected to a single extraction system.
Key changes:
- Added a
groupparameter tomaw_from_dffunction for specifying well grouping - Added support for skin parameters (
radius_skinandhk_skin) for advanced well modeling - Enhanced validation to ensure consistency within well groups
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| nlmod/gwf/wells.py | Core implementation of well grouping logic and skin parameter support |
| tests/test_010_wells.py | Comprehensive test coverage for new grouping functionality and skin parameters |
nlmod/gwf/wells.py
Outdated
| # configure groups | ||
| if df.index.has_duplicates: | ||
| raise ValueError( | ||
| "The index of the DataFrame must be unique. Indexing `multipliers` would go" |
There was a problem hiding this comment.
There's a missing space in the error message. The text "would gowrong" should be "would go wrong".
| "The index of the DataFrame must be unique. Indexing `multipliers` would go" | |
| "The index of the DataFrame must be unique. Indexing `multipliers` would go " |
nlmod/gwf/wells.py
Outdated
| default is None, which means that the skin is not used. | ||
| hk_skin : str, optional | ||
| The column in df that contains the horizontal hydraulic conductivity of the skin | ||
| around the well. Only used if `condeqn` is SKIN, CUMULATIVE, or MEAN. The default |
There was a problem hiding this comment.
There's a trailing space at the end of this line in the docstring.
nlmod/gwf/wells.py
Outdated
| iw = 0 # grouped well index | ||
| for well_group_name, well_group in tqdm( | ||
| df.groupby(group_by), total=len(df), desc="Adding MAW wells", disable=silent |
There was a problem hiding this comment.
The total parameter in tqdm should reflect the number of groups being processed, not the number of wells. When wells are grouped, there will be fewer iterations than len(df). This could cause the progress bar to be inaccurate.
| iw = 0 # grouped well index | |
| for well_group_name, well_group in tqdm( | |
| df.groupby(group_by), total=len(df), desc="Adding MAW wells", disable=silent | |
| n_groups = group_by.nunique() | |
| iw = 0 # grouped well index | |
| for well_group_name, well_group in tqdm( | |
| df.groupby(group_by), total=n_groups, desc="Adding MAW wells", disable=silent |
nlmod/gwf/wells.py
Outdated
| # [wellno, radius, bottom, strt, condeqn, ngwfnodes] | ||
| if strt is None: | ||
| if isinstance(irow["cellid"], (np.integer, int)): | ||
| if pd.api.types.is_integer_dtype(df.cellid): |
There was a problem hiding this comment.
This check uses df.cellid but should use well_group['cellid'] to check the data type for the current group. Using the entire dataframe could give incorrect results when the group contains mixed data types.
| if pd.api.types.is_integer_dtype(df.cellid): | |
| if pd.api.types.is_integer_dtype(well_group["cellid"]): |
|
The errors in the tests are unrelated |
|
Accidentally clicked on the copilot button and thereby asking for it to review the PR.. It actually did a good job here ;) Ready to be merged as far as I am concerned |
OnnoEbbens
left a comment
OnnoEbbens
left a comment
There was a problem hiding this comment.
Haha, nice. Copilot took my job!
The function maw_from_df now has the additional argument group, that allows for grouping of wells. This might be of use if a single pump is used to extract water from multiple wells. These individual wells may be placed in different cells, in different layers, but have to have the same radius.
Note that boundnames have to be the same for every well within a group, which makes sense but will be a common pitfall.