Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

silence HantushWellModel info msg about r=1.0 #604

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Conversation

dbrakenhoff
Copy link
Member

@dbrakenhoff dbrakenhoff commented Jun 6, 2023

This PR silences the info message internally by adding a warn keyword argument and checking whether the current rfunc is HantushWellModel in ps.Model methods.

In my opinion this is a bit ugly, but perhaps worth silencing the confusing message? So far 3 issues have been posted about it, so it is confusing people.

Checklist before PR can be merged:

- add warn keyword argument to methods
- add if/else to determine whether rfunc is HantushWM
@codacy-production
Copy link

Coverage summary from Codacy

Merging #604 (e769550) into dev (f8b0771) - See PR on Codacy

Coverage variation Diff coverage
-0.05% (target: +0.00%) 70.97%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (f8b0771) 4955 3599 72.63%
Head commit (e769550) 4976 (+21) 3612 (+13) 72.59% (-0.05%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#604) 31 22 70.97%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@raoulcollenteur
Copy link
Member

@dbrakenhoff , thanks for the PR. I see that fixing this would be nice.

I thought of another (easyier?) fix. Can we not deal with the distance=None issue when creating a WellModel instance? An assign r=1 if no distances are provided? That would fix it as well write? It seems to me that r should not be None in all cases anyway.

Cheers,
Raoul

@dbrakenhoff
Copy link
Member Author

That could work, and we could set the default value to 1 m or the distance to the first well. But that would mean users would have to be aware that when using ml.<method> some assumption is being made about which distance is used. I'm not sure if a silent assumption is the way to go here.

I think my preference is this PR, which provides a warning when users call ml.<method> on WellModels directly, but otherwise stays silent. But I'm open to others opinions/votes on this issue :).

@raoulcollenteur
Copy link
Member

I agree that it's better to make there users aware and raise a warning. Still, could we not do that when creating the stress model without a distance and setting r to 1, and raising the warning once upon creation? Would prevent a lot of code and it would not be a silent assumption right?

@dbrakenhoff
Copy link
Member Author

I agree that it's better to make there users aware and raise a warning. Still, could we not do that when creating the stress model without a distance and setting r to 1, and raising the warning once upon creation? Would prevent a lot of code and it would not be a silent assumption right?

The trouble is that the warning isn't likely to be generated at creation, but when users want to use the Model class convenience methods for getting information from the stressmodels (i.e. ml.get_step_response). So for now, I think this is the best solution, even though it does add some extra code.

@raoulcollenteur raoulcollenteur added this to the 1.2 milestone Aug 15, 2023
@raoulcollenteur raoulcollenteur added the bug Indicates an unintended behavior or coding error label Aug 15, 2023
@raoulcollenteur raoulcollenteur merged commit aee5246 into dev Aug 15, 2023
8 of 10 checks passed
@raoulcollenteur raoulcollenteur deleted the 602-silence-hwm branch August 15, 2023 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unintended behavior or coding error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants