Use istress in responses in result-plot #714
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesYou may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation |
pastas/model.py
Outdated
|
|
||
| @get_stressmodel | ||
| def get_step_response( | ||
| self, | ||
| name, | ||
| p: Optional[ArrayLike] = None, |
There was a problem hiding this comment.
Why are these arguments removed? I don't think this is necessary and this should be retained. Same for get_block_response.
There was a problem hiding this comment.
There is no need to specifically name these arguments, as the kwargs are already passed onto _get_response, but I will change it back. May be easier for someone to see these parameters directly.
There was a problem hiding this comment.
I was specifically thinking about the case when people use this method and want to change the input, e.g., I use add0 every now and then so it's nice that these features are documented.
There was a problem hiding this comment.
They are still named in the docstring, so they are documented. However, I have just added them again as arguments as well.
| @@ -1520,6 +1520,36 @@ def get_water_balance( | |||
| df.index = self.prec.series.index | |||
| return df | |||
|
|
|||
| def get_parameters(self, model=None, istress: Optional[int] = None) -> ArrayLike: | |||
There was a problem hiding this comment.
Let's make this method private, ._get_parameters. I don't think users should use this method to get the parameters, because it is very specific for the linear model to get adapted parameters for the response function. So only for internal use.
There was a problem hiding this comment.
I will also change WellMode.get_parameters to WellMode._get_parameters. Is this ok @dbrakenhoff ?
There was a problem hiding this comment.
I'm not sure. I use it quite a bit, because quite a few of the Model class' methods do not work out-of-the-box for the WellModel. In order to get the distance added to the 3 Hantush parameters, I use p = WellModel.get_parameters(model=ml), and pass that onto any model class methods, e.g. ml.get_step_response("wells", p=p).
I think in this case, this method is also useful for other users, and not just for internal use.
EDIT: I still think this isn't a very pretty solution, but I can't think of a better one.
There was a problem hiding this comment.
Then I am in favor of adding a get_parameters to all stressmodels (through StressModelBase), which would return the optimal parameters when a model is supplied and the initial values otherwise.
|
Looks good! I suggested a few small changes to ensure users know the arguments and not to use methods that I think are only for internal usage. Do we know if split=True is still used a lot? |
I use it. Do not know of other people. |
|
@rubencalje can you resolve the conflict? Then we can merge this one I think? |
This can be merged now. |
Short Description
This PR improves the response-plots in ml.plots.results() when the influence of different stresses in a single stressmodel are plotted:
ml.plots.results(split-True). This can be used in the stressmodel RechargeModel with recharge = Linear and in the stressmodel WellModel.The old behavior would result in the following figure (see the gap on the right and the positive response for evaporation):
While the new behavior results in the following figure (see the different response for recharge and evaporation):
In this implementation, I added a
get_parametersmethod for RechargeModel, in the same manner already existed for WellModel.Checklist before PR can be merged: