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

Do not add a noisemodel to a Model by default anymore #667

Closed
raoulcollenteur opened this issue Nov 29, 2023 · 6 comments
Closed

Do not add a noisemodel to a Model by default anymore #667

raoulcollenteur opened this issue Nov 29, 2023 · 6 comments
Assignees
Labels
development Indicates development of new features priority 1 normal, deal with in the foreseeable future

Comments

@raoulcollenteur
Copy link
Member

raoulcollenteur commented Nov 29, 2023

A discussed in the last Pastas meeting, we plan to make noise=False the new default for ml.solve for Pastas 2.0 release. This Issue is to document the work related to this big change. Here are some of the reasons why we plan to make this change:

  • This way we don't force users to use a noise model when they don't need it.
  • This will improve the calibration results in terms of goodness-of-fit for many beginning users.
  • Some solve methods don't use a noise model anymore. This is for example already the case when using MCMC solvers and a likelihood function that accounts for autocorrelated errors.
  • If users need a noise model for any reason this will be a more conscious choice and still be possible.
  • Other methods for UQ with autocorrelated errors are explored.
  • .... ?

To do list related to this Issue:

[ ] Investigate alternative methods for uncertainty quantification with autocorrelated errors.
[ ] Document this change throughout pastas
[ ] Provide FutureWarnings in the last version before 2.0 (whenever that is).
[ ] ...?

@raoulcollenteur raoulcollenteur added the development Indicates development of new features label Nov 29, 2023
@raoulcollenteur raoulcollenteur added this to the 2.0: Bolognese milestone Nov 29, 2023
@raoulcollenteur
Copy link
Member Author

A similar warning as to Pandas could be given:

FutureWarning: The default of observed=False is deprecated and will be changed to True in a future version of pandas. Pass observed=False to retain current behavior or observed=True to adopt the future default and silence this warning.

@rubencalje
Copy link
Collaborator

One question we need to answer: what do we do with the parameter uncertainty, for example the standard error in the fit report, when noise is set to False? The uncertainty will likely be underestimated, as there will be much autocorrelation in the residuals. There are several options:

  • just display the standard error (like we do now)
  • do not display the standard error
  • display the standard error in another way (Italic type or in orange/red)

I will start a branch to make some of the needed changes, so we can use this for testing.

@mbakker7
Copy link
Collaborator

Good question. I think I am in favor of not displaying the standard error anymore whether the noise model is used or not. After all, just using it doesn't necessarily mean the autocorrelation is gone and so the standard error may still not be representative. Then we should probably create a separate function to compute the standard error.

@martinvonk martinvonk added the priority 1 normal, deal with in the foreseeable future label Feb 19, 2024
@rubencalje
Copy link
Collaborator

Comment from @mbakker7 in PR #678 (comment):

For version 2.0, I think the default should be to add a NoiseModel like adding a stress model. That way different noise models can be added. So my suggestion for version 2.0 is that ml.solve can not add a noise model. Come to think of it, this may have been the original implementation of a noise model in the first versions of pastas.

@raoulcollenteur raoulcollenteur changed the title [DEVELOPMENT] Make noise=False the new default in ml.solve for Pastas 2.0 Make noise=False the new default in ml.solve for Pastas 2.0 Apr 9, 2024
@rubencalje
Copy link
Collaborator

rubencalje commented Apr 10, 2024

As discussed at the meeting of 2024-4-9, we will not add a noisemodel to a Model object by default anymore. If a noisemodel is required, the user can add it using ml.add_noisemodel(ps.NoiseModel()). See PR #678.

In Pastas 2.0 we will also remove the noisemodel argument from the Model class, and the noise argument from Model.solve().

@rubencalje rubencalje changed the title Make noise=False the new default in ml.solve for Pastas 2.0 Do not add a noisemodel to a Model by default anymore Apr 10, 2024
@raoulcollenteur
Copy link
Member Author

As of Pastas version 1.5, no noisemodel is added by default anymore. For more information about this change and how to add a noise model if desired, see Issue #735.

Closing issue after merging PR #678 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Indicates development of new features priority 1 normal, deal with in the foreseeable future
Projects
None yet
Development

No branches or pull requests

5 participants