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.

Sign up for GitHub

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

Jump to bottom

[ENHANCEMENT] up argument ignored in response function #551

Closed
dbrakenhoff opened this issue Feb 14, 2023 · 3 comments · Fixed by #573
Closed

[ENHANCEMENT] up argument ignored in response function #551

dbrakenhoff opened this issue Feb 14, 2023 · 3 comments · Fixed by #573
Assignees
@dbrakenhoff
Labels
enhancement Indicates improvement of existing features priority 0 high, deal with as soon as possible
Milestone
1.1

Comments

@dbrakenhoff
Copy link
Member

Describe the proposed enhancement
The up value passed to a response function is ignored, and overwritten by the StressModel up value.

This works:

sm = ps.StressModel(stress, ps.Gamma(), "well", up=False)
sm.rfunc.up  # -> False

This does not:

sm = ps.StressModel(stress, ps.Gamma(up=False), "well")
sm.rfunc.up  # -> True

So my question is, do we want the second option to work as well?

I think it would make sense if both options work, so I'd expect StressModel to defer to rfunc's default.

We can implement this by setting up=None in StressModelBase, and then deferring to RfuncBase default up value when that is the case. If up=True/False is passed to the StressModel, that boolean value will be used. The downside is that the StressModel will show up=None, which is slightly less informative for users.

Thoughts?
@dbrakenhoff dbrakenhoff added the enhancement Indicates improvement of existing features label Feb 14, 2023
@martinvonk martinvonk added the priority 0 high, deal with as soon as possible label Mar 7, 2023
@martinvonk
Copy link
Collaborator

martinvonk commented Mar 21, 2023 •
edited

We'll remove the up attribute from the RfuncBase class.

@raoulcollenteur
Copy link
Member

Related to #485

dbrakenhoff added a commit that referenced this issue Mar 21, 2023
@dbrakenhoff
fix #551
4f0c464 
- up and gain_scale_factor arguments removed from RfuncBase init
- attributes have default values up=None and gain_scale_factor=1.0
- attributes are updated by StressModels using update_rfunc_settings()
- expose method update_rfunc_settings for external use (e.g. for setting up and gain_scale_factor for synthetically generated time series).
@dbrakenhoff dbrakenhoff mentioned this issue Mar 21, 2023
Merged
5 tasks
@dbrakenhoff dbrakenhoff linked a pull request Mar 21, 2023 that will close this issue
fix #551: up and gain_scale_factor are set by StressModel #573
Merged
5 tasks
raoulcollenteur pushed a commit that referenced this issue May 4, 2023
@dbrakenhoff
fix #551: up and gain_scale_factor are set by StressModel (#573)
aac3f9d
@raoulcollenteur raoulcollenteur added this to the 1.1 milestone May 4, 2023
@dbrakenhoff
Copy link
Member Author

Solved by #551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dbrakenhoff dbrakenhoff

Labels
enhancement Indicates improvement of existing features priority 0 high, deal with as soon as possible
Projects
None yet
Milestone
1.1
3 participants
@dbrakenhoff @raoulcollenteur @martinvonk