Do not add NoiseModel to Model-class by default anymore #678

rubencalje · 2024-01-19T09:19:02Z

Short Description

This Pull Request does not add a noisemodel to the Model class by default anymore.

See issue #735 for more information.

Checklist before PR can be merged:

one of the needed changes to fix issue Do not add a noisemodel to a Model by default anymore #667
is documented
Format code with Black formatting
type hints for functions and methods
tests added / passed
Example Notebook (for new features)
Remove output for all notebooks with changes

@raoulcollenteur

Based on proposal from @raoulcollenteur in issue #667

codacy-production · 2024-01-19T09:28:57Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.13% (target: +0.00%)	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`9c76689`)	5833	4358	74.71%
Head commit (`6ceb401`)	5859 (+26)	4385 (+27)	74.84% (+0.13%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#678)	3	3	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

codacy-production · 2024-01-19T09:28:57Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.13% (target: +0.00%)	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`9c76689`)	5833	4358	74.71%
Head commit (`183c542`)	5859 (+26)	4385 (+27)	74.84% (+0.13%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#678)	3	3	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

where necessary to get same output

raoulcollenteur · 2024-01-31T11:00:07Z

Should we merge this for 1.4 or wait one version? Giving a warning early about this future change might be good?

rubencalje · 2024-01-31T12:14:59Z

Should we merge this for 1.4 or wait one version? Giving a warning early about this future change might be good?

I agree, an early warning is good. I am in favor of merging before the next version.

mbakker7 · 2024-01-31T14:15:34Z

This is a big change (although I am all for it). As this will give different results with the default values for many models, I think it warrants a major version number change. Should we go to 2.0 for this?
Regarding a warning, what warning would you like to give. Every time ml.solve is called warn the user that in the future (2.0) the default will be noise=False? That would surely get annoying and we shouldn't do that for too many versions.

dbrakenhoff · 2024-01-31T14:35:38Z

Perhaps a global "I got it, don't keep warning me" option would be nice to include, to avoid repeating the warning so often people stop reading it?

mbakker7 · 2024-01-31T14:48:45Z

Perhaps a global "I got it, don't keep warning me" option would be nice to include, to avoid repeating the warning so often people stop reading it?

Any thought on how to implement this? You want to give this warning once after the first solve after a Pastas import?
I am mostly worried that people will turn warnings off. Which is what I would do in their case as well.

dbrakenhoff · 2024-01-31T15:14:28Z

Any thought on how to implement this? You want to give this warning once after the first solve after a Pastas import?
I am mostly worried that people will turn warnings off. Which is what I would do in their case as well.

Maybe a global boolean setting that is flipped after one solve call? And can be turned off manually as well? And turning it off would then mean your code includes something like ps.turn_off_default_noise_false_warning() which indicates some awareness of the future change in default behavior? But I haven't completely thought it through yet, it was just an idea that popped up as I read the previous comments.

rubencalje · 2024-02-27T10:05:21Z

People can just set noise=True in their code, which is what we want them to do. Implementing a global boolean or something else to bypass this warning will make them not set noise=True in their code, which will then give different results in the future. So I am not in favor of hiding the warning.

martinvonk · 2024-03-06T15:36:36Z

I found this on stackoverflow so that you can raise warnings and log them without duplicating the message: logging.captureWarnings(True)

martinvonk · 2024-03-07T09:32:15Z

Related to the comment above: this works for me in #700 to raise a FutureWarning in the logger:

from logging import captureWarnings, getLogger
from warnings import warn

captureWarnings(True)
logger = getLogger(__name__)
warnings_logger = getLogger("py.warnings")
logger.addHandler(warnings_logger)

warn(
    "Type Your Message Here",
    category=FutureWarning,
)

codacy-production · 2024-03-15T09:40:48Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.33% (target: +0.00%)	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`a2e178b`)	6084	4580	75.28%
Head commit (`e6b5266`)	6137 (+53)	4640 (+60)	75.61% (+0.33%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#678)	10	10	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

pastas/model.py

mbakker7 · 2024-03-19T19:13:10Z

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 · 2024-03-29T08:04:36Z

@rubencalje, can you implement the same for ml = Model(noisemodel=True) and set the default to False? Then no noisemodel is silently added by default anymore and users have to be explicit there as well.

rubencalje · 2024-03-29T09:04:42Z

@rubencalje, can you implement the same for ml = Model(noisemodel=True) and set the default to False? Then no noisemodel is silently added by default anymore and users have to be explicit there as well.

I can do that, but I think we then need to change the default behavior of the noise-parameter in ml.solve() to: if you have added a noisemodel, solve with noise = True, and if you have not added a noisemodel, solve with noise = False.

We then remove the warning implemented now, and add a similar warning to the __init__ of the Model-class.

And change ml.solve(noise=True) to ml = ps.Model(noisemodel=True) in many examples.

…tests

plus add warning to top of docs plus add warning on import

codacy-production · 2024-04-11T14:38:15Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.01% (target: +0.00%)	✅ 68.97%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`bf64e28`)	6163	4647	75.40%
Head commit (`a7b4223`)	6242 (+79)	4707 (+60)	75.41% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#678)	87	60	68.97%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

raoulcollenteur · 2024-04-11T19:01:08Z

This PR is ready for review again.

rubencalje · 2024-04-12T07:33:07Z

I made some changes to the messages that I think will inform the user a bit better.

And saved before version 1.5

…stas into prepare_noise_is_false

rubencalje · 2024-04-12T08:43:34Z

Made a change so that loading of pas-files from versions before 1.5 that were solved with ml.solve(noise=False) can be loaded and will give the same results, by removing the noisemodel from these models. PR can be merged.

I added a few words and the Github issue with clarification.

mbakker7 · 2024-04-12T09:20:26Z

Before putting this in dev, I think @martinvonk and @OnnoEbbens should merge their PRs (#732 and #729) in this PR first.

change capslock to only capital for first letter

Changes NoiseModel -> ArNoiseModel ArmaModel -> ArmaNoiseModel

Co-authored-by: Raoul Collenteur <raoulcollenteur@gmail.com> Co-authored-by: Davíd Brakenhoff <d.brakenhoff@artesia-water.nl>

@martinvonk

…he future With permission of @martinvonk !

mbakker7

I think this is ready to go.
Big but important step forward.

Add warning when noise is not set in ml.solve

6ceb401

Based on proposal from @raoulcollenteur in issue #667

github-actions bot assigned rubencalje Jan 19, 2024

rubencalje changed the base branch from master to dev January 19, 2024 09:19

Add noise=True in notebooks and examples

183c542

where necessary to get same output

raoulcollenteur approved these changes Jan 31, 2024

View reviewed changes

Merge branch 'dev' into prepare_noise_is_false

9e614e6

rubencalje marked this pull request as ready for review March 15, 2024 09:30

raoulcollenteur added this to the 1.5 milestone Mar 19, 2024

raoulcollenteur reviewed Mar 19, 2024

View reviewed changes

pastas/model.py Outdated Show resolved Hide resolved

pastas/model.py Outdated Show resolved Hide resolved

raoulcollenteur added enhancement Indicates improvement of existing features development Indicates development of new features labels Mar 19, 2024

rubencalje mentioned this pull request Mar 21, 2024

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

Closed

rubencalje added 3 commits April 3, 2024 15:56

Set default value of noisemodel to None in Model

9e4ffbd

And change ml.solve(noise=True) to ml = ps.Model(noisemodel=True) in many examples.

Set noisemodel from None before settings, update docstrings and some …

9f12f4d

…tests

Edit notebooks in benchmarks and userguide

bd303b2

raoulcollenteur added 3 commits April 11, 2024 16:00

Raise appropriate errors if noisemodel/noise is used

0f8a094

plus add warning to top of docs plus add warning on import

Update __init__.py

d5dbcb3

Update notebooks to new behavior

54eb2b7

martinvonk and others added 2 commits April 11, 2024 17:13

Merge branch 'prepare_noise_is_false' into noisemodel_names

1f71c06

format message on import

cd3a3c7

raoulcollenteur requested a review from dbrakenhoff April 11, 2024 18:42

Some changes to the messages to the user

24f0daf

rubencalje and others added 4 commits April 12, 2024 10:00

Change "silence this warning" to "fix this error"

ba9a37a

Fix loading of model that was solved with ml.solve(noise=False)

108d07d

And saved before version 1.5

fixed typos and added reference to github issue

8e4c057

Merge branch 'prepare_noise_is_false' of https://github.com/pastas/pa…

cbc433c

…stas into prepare_noise_is_false

some more clarification in error message

b06b77a

I added a few words and the Github issue with clarification.

mbakker7 and others added 8 commits April 12, 2024 11:32

fix typose

d5e170f

improve readability noisemodel names

a4f9f7b

change capslock to only capital for first letter

change two more instances of NoiseModel

d0a1df5

Merge branch 'prepare_noise_is_false' into noisemodel_names

c2a392f

Change NoiseModel names (#732)

ad26470

Changes NoiseModel -> ArNoiseModel ArmaModel -> ArmaNoiseModel

722 show estimated parameter uncertainties or not (#729)

c4a5e0c

Co-authored-by: Raoul Collenteur <raoulcollenteur@gmail.com> Co-authored-by: Davíd Brakenhoff <d.brakenhoff@artesia-water.nl>

Improve messages of renamed Noisemodels and fix read of pas-file in t…

a442256

…he future With permission of @martinvonk !

forgot to save file...

a7b4223

mbakker7 approved these changes Apr 13, 2024

View reviewed changes

raoulcollenteur approved these changes Apr 14, 2024

View reviewed changes

raoulcollenteur removed the request for review from dbrakenhoff April 15, 2024 05:14

raoulcollenteur merged commit 8ec2944 into dev Apr 15, 2024
11 of 12 checks passed

raoulcollenteur deleted the prepare_noise_is_false branch April 15, 2024 05:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not add NoiseModel to Model-class by default anymore #678

Do not add NoiseModel to Model-class by default anymore #678

rubencalje commented Jan 19, 2024 •

edited

Loading

codacy-production bot commented Jan 19, 2024

codacy-production bot commented Jan 19, 2024 •

edited

Loading

raoulcollenteur commented Jan 31, 2024

rubencalje commented Jan 31, 2024

mbakker7 commented Jan 31, 2024

dbrakenhoff commented Jan 31, 2024

mbakker7 commented Jan 31, 2024

dbrakenhoff commented Jan 31, 2024

rubencalje commented Feb 27, 2024

martinvonk commented Mar 6, 2024

martinvonk commented Mar 7, 2024 •

edited

Loading

codacy-production bot commented Mar 15, 2024 •

edited

Loading

mbakker7 commented Mar 19, 2024

raoulcollenteur commented Mar 29, 2024

rubencalje commented Mar 29, 2024

codacy-production bot commented Apr 11, 2024 •

edited

Loading

raoulcollenteur commented Apr 11, 2024

rubencalje commented Apr 12, 2024

rubencalje commented Apr 12, 2024

mbakker7 commented Apr 12, 2024 •

edited by martinvonk

Loading

mbakker7 left a comment

Do not add NoiseModel to Model-class by default anymore #678

Do not add NoiseModel to Model-class by default anymore #678

Conversation

rubencalje commented Jan 19, 2024 • edited Loading

Short Description

Checklist before PR can be merged:

codacy-production bot commented Jan 19, 2024

Coverage summary from Codacy

codacy-production bot commented Jan 19, 2024 • edited Loading

Coverage summary from Codacy

raoulcollenteur commented Jan 31, 2024

rubencalje commented Jan 31, 2024

mbakker7 commented Jan 31, 2024

dbrakenhoff commented Jan 31, 2024

mbakker7 commented Jan 31, 2024

dbrakenhoff commented Jan 31, 2024

rubencalje commented Feb 27, 2024

martinvonk commented Mar 6, 2024

martinvonk commented Mar 7, 2024 • edited Loading

codacy-production bot commented Mar 15, 2024 • edited Loading

Coverage summary from Codacy

mbakker7 commented Mar 19, 2024

raoulcollenteur commented Mar 29, 2024

rubencalje commented Mar 29, 2024

codacy-production bot commented Apr 11, 2024 • edited Loading

Coverage summary from Codacy

raoulcollenteur commented Apr 11, 2024

rubencalje commented Apr 12, 2024

rubencalje commented Apr 12, 2024

mbakker7 commented Apr 12, 2024 • edited by martinvonk Loading

mbakker7 left a comment

Choose a reason for hiding this comment

rubencalje commented Jan 19, 2024 •

edited

Loading

codacy-production bot commented Jan 19, 2024 •

edited

Loading

martinvonk commented Mar 7, 2024 •

edited

Loading

codacy-production bot commented Mar 15, 2024 •

edited

Loading

codacy-production bot commented Apr 11, 2024 •

edited

Loading

mbakker7 commented Apr 12, 2024 •

edited by martinvonk

Loading