-
-
Notifications
You must be signed in to change notification settings - Fork 728
Introduce @deprecated decorators for functions and enforce warnings as errors in tests #4757
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
base: develop
Are you sure you want to change the base?
Changes from 37 commits
ff6a817
7f34a0b
3da7140
3a15fe8
9c500b5
949571b
3f327cb
9d29350
1eb61a0
47051ab
e57a1fd
dba1881
c48ed38
b91e9ce
c564c87
52e610b
a4025a1
33ff787
9d9c1bf
139704c
808d5cd
28f5397
a703eca
8784b35
7902c43
eaf0994
930190b
6c76623
422029a
6e2eb5a
840b696
194c657
41e58e3
a565419
0bf9abf
6a63a05
444cf08
6db0714
2e8d684
71a5197
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ dependencies = [ | |
| "posthog", | ||
| "pyyaml", | ||
| "platformdirs", | ||
| "deprecation" | ||
| ] | ||
|
|
||
| [project.urls] | ||
|
|
@@ -80,7 +81,7 @@ plot = [ | |
| "matplotlib>=3.6.0", | ||
| ] | ||
| cite = [ | ||
| "setuptools", # Fix for a pybtex issue | ||
| "setuptools==66.1.1", # Fix for a pybtex issue | ||
| "pybtex>=0.24.0", | ||
| ] | ||
| # Battery Parameter eXchange format | ||
|
|
@@ -231,8 +232,10 @@ required_plugins = [ | |
| addopts = [ | ||
| "-nauto", | ||
| "-vra", | ||
| "--showlocals", | ||
| "--strict-config", | ||
| "--strict-markers", | ||
| "--ignore=tests/unit/test_parameters/test_bpx.py", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than ignoring the file completely, let's skip/xfail the relevant tests in that file. As I previously mentioned: if you can't address/capture the warnings coming out of them, please open an issue in the BPX repo and link it here. |
||
| ] | ||
| testpaths = [ | ||
| "tests", | ||
|
|
@@ -244,10 +247,14 @@ filterwarnings = [ | |
| # ignore internal nbmake warnings | ||
| 'ignore:unclosed \<socket.socket:ResourceWarning', | ||
| 'ignore:unclosed event loop \<:ResourceWarning', | ||
| # ignore warnings generated while running tests | ||
| "ignore::DeprecationWarning", | ||
| # convert internal warnings as errors | ||
| "error::DeprecationWarning:pybamm.*", | ||
RohitP2005 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| # ignore setuptools and pybtex deprecation warnings | ||
| "ignore::DeprecationWarning:pkg_resources", | ||
| "ignore:open_text is deprecated:DeprecationWarning", | ||
| "ignore::UserWarning", | ||
| "ignore::RuntimeWarning", | ||
| "ignore::pydantic.warnings.PydanticDeprecatedSince20", | ||
| ] | ||
|
|
||
| # Logging configuration | ||
|
|
||
RohitP2005 marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,45 +67,6 @@ def test_symbol_domains(self): | |
| with pytest.raises(NotImplementedError, match="Cannot set domain directly"): | ||
| b.domain = "test" | ||
|
|
||
| def test_symbol_auxiliary_domains(self): | ||
| a = pybamm.Symbol( | ||
| "a", | ||
| domain="test", | ||
| auxiliary_domains={ | ||
| "secondary": "sec", | ||
| "tertiary": "tert", | ||
| "quaternary": "quat", | ||
| }, | ||
| ) | ||
| assert a.domain == ["test"] | ||
| assert a.secondary_domain == ["sec"] | ||
| assert a.tertiary_domain == ["tert"] | ||
| assert a.tertiary_domain == ["tert"] | ||
| assert a.quaternary_domain == ["quat"] | ||
| assert a.domains == { | ||
| "primary": ["test"], | ||
| "secondary": ["sec"], | ||
| "tertiary": ["tert"], | ||
| "quaternary": ["quat"], | ||
| } | ||
|
|
||
| a = pybamm.Symbol("a", domain=["t", "e", "s"]) | ||
| assert a.domain == ["t", "e", "s"] | ||
| with pytest.raises(TypeError): | ||
| a = pybamm.Symbol("a", domain=1) | ||
| b = pybamm.Symbol("b", domain="test sec") | ||
| with pytest.raises(pybamm.DomainError, match="All domains must be different"): | ||
| b.domains = {"primary": "test", "secondary": "test"} | ||
| with pytest.raises(pybamm.DomainError, match="All domains must be different"): | ||
| b = pybamm.Symbol( | ||
| "b", | ||
| domain="test", | ||
| auxiliary_domains={"secondary": ["test sec"], "tertiary": ["test sec"]}, | ||
| ) | ||
|
|
||
| with pytest.raises(NotImplementedError, match="auxiliary_domains"): | ||
| a.auxiliary_domains | ||
|
|
||
|
||
| def test_symbol_methods(self): | ||
| a = pybamm.Symbol("a") | ||
| b = pybamm.Symbol("b") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious: is there a reason you think we should opt for this option? Do you feel that our tracebacks are limited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conversation is still relevant, as I'm not sure why we need to include this option with this PR – is it related to the changes? It's useful, but if it doesn't need to be included for the main changes here, please consider adding it in another PR.