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

[ENHANCEMENT] where to set cutoff, gain_scale_factor and up? #485

Closed
raoulcollenteur opened this issue Jan 25, 2023 · 3 comments
Closed
Assignees
Labels
enhancement Indicates improvement of existing features
Milestone

Comments

@raoulcollenteur
Copy link
Member

raoulcollenteur commented Jan 25, 2023

Describe the proposed enhancement

After the change to only allow instances of response functions, a user can set the gain_scale_factor, up, and cutoff in two places: ps.Gamma(arg=arg) or ps.StressModel(ps.Gamma(), arg=arg). The args given to StressModel will override those from the response function. I don't think this is the desired behaviour.

  • So to start, I would propose to remove the cutoff argument from the StressModels and only allow setting this parameter when creating a response function instance.
  • For up, intuitively I think it makes more sense to set this at the rfunc instance level and looking at the code I can only see a few small changes for WellModel required, which has up=True by default.
  • For gain_scale_factor, this depends on the stress and I think it makes sense to set this through the StressModel and keep this as is. However, for this argument, only the StressModel actually allows setting it and all other stressmodels set this values internally, which is also not what we want I think. Here I prefer the StressModel behaviour, only using stress.std() if no gain_scale_factor is provided.
@raoulcollenteur raoulcollenteur added the enhancement Indicates improvement of existing features label Jan 25, 2023
@raoulcollenteur raoulcollenteur self-assigned this Jan 25, 2023
@mbakker7
Copy link
Collaborator

My thoughts:

  1. cutoff is set in the rfunc only. So I agree.
  2. up should be set by the stress model. Only the stress model knows whether a positive stress should create a rise or decline of the head. So we need a set_up function for response functions.
  3. meanstress should be set by the stress model. Again because only the stress model knows that value. So here I agree again.

@raoulcollenteur raoulcollenteur added this to the 0.23.0 milestone Jan 25, 2023
@raoulcollenteur
Copy link
Member Author

I addressed this issue and implemented the changes as commented by @mbakker7 in PR #491 .

@raoulcollenteur raoulcollenteur changed the title [ENHANCEMENT] where to set cutoff, meanstress and up? [ENHANCEMENT] where to set cutoff, gain_scale_factor and up? Jan 26, 2023
@raoulcollenteur
Copy link
Member Author

Changes made. The new place to set cutoff is at the response function level. The other two arguments are still provided at the stressmodel level. For the gain_scale_factor, it should be noted that all stressmodels set these internally, except for StressModel, and cannot be set by the user. Whether or not that is desired behaviour could be another issue in the future.

Closing this issue now, as PR #491 has been merged and addressed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates improvement of existing features
Projects
None yet
Development

No branches or pull requests

2 participants