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

add check if noise_alpha is smaller than model freq #164

Merged
merged 1 commit into from
Nov 1, 2019

Conversation

dbrakenhoff
Copy link
Member

This small change is meant to address the issue that arises when a higher frequency oseries is used to create a model but the frequency in solve is set to 1 day (or longer). In this situation the noise_alpha parameter is too small, which can lead to the optimizer effectively "turning off" the noisemodel.

There is a case in which this change might not be ideal, but I think this is preferable over the situation described above. With this change the following could occur:

  • user creates model with oseries on freq 'H'
  • default model settings is freq 'D', so noise_alpha is set to 1.0
  • user wants to optimize on freq 'H', which means a better initial noise alpha might be 0.042.

The broader discussion here is is that the initial values of parameters are estimated based on data that might not be the same as the data that is used in ml.solve(). This is because the timeseries can be altered based on the kwargs provided to solve (i.e. freq, tmin, tmax).

Some thoughts to consider:

  • Should there be an option to (re-)estimate initial parameters in ml.initialize()?
  • Perhaps a column could be added to ml.parameters to indicate whether parameters' initial values were manually adjusted? In that case we can re-estimate the non-adjusted parameters based on oseries_calib and leave the rest as the user intended...

@dbrakenhoff dbrakenhoff added the enhancement Indicates improvement of existing features label Nov 1, 2019
@dbrakenhoff dbrakenhoff added this to the 0.13.0 milestone Nov 1, 2019
@raoulcollenteur
Copy link
Member

I'll merge this PR now. I think depending on how we deal with the "things to think about" you mentioned we might need to change it later but this seems to work okay for now.

@raoulcollenteur raoulcollenteur merged commit a3b64cb into dev Nov 1, 2019
@raoulcollenteur raoulcollenteur deleted the noise_alpha branch March 14, 2020 18:29
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

Successfully merging this pull request may close these issues.

None yet

2 participants