fix: Add warning when existing parameters are updated

[kratman]

Description

I was looking at #2265 and saw that a lot of tests (34) and examples (22) would be affected by making it an error to update parameters with check_already_exists=False. This makes it seem like a larger change. As an intermediate I propose to implement a warning so users and developers can adjust their scripts before it becomes an error. Similar to adding a deprecation warning.

If this change makes it in before the next release, then users can start adapting their code before the errors begin.

Related #2265

Type of change

Minor change to the way parameters are updated.

• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a129e02) 99.59% compared to head (3094930) 99.59%.
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3382   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        20755    20757    +2     
========================================
+ Hits         20670    20672    +2     
  Misses          85       85           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kratman]

Lychee error is not from this change. Currently looking into the benchmark regression to see if it has to do with the logging.

[brosaplanella]

Lychee error is not from this change. Currently looking into the benchmark regression to see if it has to do with the logging.

Looks like a small regression which is probably random, not too worried about it.

[brosaplanella]

Looks good to me, thanks!

[kratman]

Looks like a small regression which is probably random, not too worried about it.

Yeah I figured it was probably random since the test was so simple. Looking into was more of an exercise to learn how to run them locally. Takes forever on my laptop

[valentinsulzer]

Thanks, I agree a warning is better than an error for this.
I would say let's hold off on this for the 23.9 release so we can test it a bit and see how annoying the warning is.
Also only tangentially related, we have a few of these kinds of settings now where the user could choose a different default option, do you know a nice way of making a "global" settings file for a pip-installed package?

[kratman]

Thanks, I agree a warning is better than an error for this. I would say let's hold off on this for the 23.9 release so we can test it a bit and see how annoying the warning is.

If the warning is unwanted, I can just start working on option (2) that I suggested in the original issue. I only intended the warning to be a temporary measure until the larger breaking change was implemented.

[agriyakhetarpal]

a nice way of making a "global" settings file for a pip-installed package

One of the recommended ways for this is to use configuration files that are parsed at the initialisation of a class (in this case, the Settings class) or at the time of import: it would be good to have something in either INI, TOML, YAML or JSON format that can be kept git-ignored for and by users, and otherwise without its existence we can use the defaults for a particular setting (in this case, displaying a warning on updating existing parameters). On pip installations (users), the file can live in the $HOME directory (or, preferably in the PyBaMM root directory because we can provide an API to read and write it)

Some examples are:
pytest: pytest.ini
coverage: .coveragerc
tox: tox.ini

The easiest of file formats to parse would be JSON (no external dependencies), though, my personal preference has always been TOML due to its readability (note: requires toml which is included in Python 3.11+ and is read-only, requires external dependencies for writing). For YAML, we already get pyyaml installed (required by both jupyter and pybtex in the extras). In our case, we provide an extensive API with programmatic access rather than a CLI application, which means that providing write access to such a settings file would be essential (it is difficult to override some of these settings without a CLI as easily).

A minimal example could be like

[pybamm.plotting]
# override, use six words instead of four
max_words_in_line = 6

It would also provide fine-grained control because of how the TOML format is defined: a table can be expressed and nested too:

[pybamm.smoothing]
tolerances.j0__c_e = 1e-8
tolerances.j0__c_s = 1e-8

is the same as

[pybamm.smoothing.tolerances]
j0__c_e = 1e-8
j0__c_s = 1e-8

and the Settings class can just read the TOML instead of hardcoding these values inside its Python file. If such functionality is required at some point, we could open an issue based on this comment

[kratman]

I am going to close this for now and revisit this ticket later. I am still assigned to the issue as a reminder