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

Translate warnings to Python #1201

Merged
merged 8 commits into from
Mar 4, 2022

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Feb 19, 2022

Changes proposed in this pull request

  • Translate warnings raised in C++ to Python warnings: warn_deprecated is raised as DeprecationWarning and warn_user is raised as UserWarning (on C++ the output is changed from CanteraWarning to UserWarning for consistency)
  • Update year in License

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#136

If applicable, provide an example illustrating new features this pull request is introducing

>>> import cantera as ct
>>> gas = ct.Solution("h2o2.cti")
<stdin>:1: DeprecationWarning: XML_Node::build: 
The CTI and XML input file formats are deprecated and will be removed in
Cantera 3.0. Use 'cti2yaml.py' or 'ctml2yaml.py' to convert CTI or XML input
files to the YAML format. See https://cantera.org/tutorials/legacy2yaml.html
for more information.

and

>>> import warnings
>>> import cantera as ct
>>> with warnings.catch_warnings():
...     warnings.simplefilter("ignore")
...     gas = ct.Solution("gri30.cti")
... 
>>> gas
<cantera.composite.Solution object at 0x7f630d950dc0>

Another nice thing is that users will be pointed to the line in existing Python code where the warning is invoked, e.g.

In [1]: %run custom_reactions.py
/work/GitHub/cantera/interfaces/cython/cantera/examples/kinetics/custom_reactions.py:34: DeprecationWarning: XML_Node::build: 
The CTI and XML input file formats are deprecated and will be removed in
Cantera 3.0. Use 'cti2yaml.py' or 'ctml2yaml.py' to convert CTI or XML input
files to the YAML format. See https://cantera.org/tutorials/legacy2yaml.html
for more information.

(this is from an instance where a DeprecationWarning in an example was previously not caught by the test suite; this is also addressed in this PR)

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@ischoegl ischoegl requested a review from a team February 19, 2022 04:24
@codecov
Copy link

codecov bot commented Feb 19, 2022

Codecov Report

Merging #1201 (4a6c372) into main (0dba120) will decrease coverage by 0.02%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1201      +/-   ##
==========================================
- Coverage   65.41%   65.38%   -0.03%     
==========================================
  Files         318      318              
  Lines       46085    46095      +10     
  Branches    19604    19604              
==========================================
- Hits        30145    30141       -4     
- Misses      13426    13439      +13     
- Partials     2514     2515       +1     
Impacted Files Coverage Δ
include/cantera/cython/wrappers.h 85.15% <50.00%> (-0.33%) ⬇️
include/cantera/base/global.h 81.81% <60.00%> (-2.40%) ⬇️
src/base/application.cpp 63.98% <83.33%> (-1.26%) ⬇️
include/cantera/base/logger.h 78.57% <100.00%> (+5.84%) ⬆️
src/base/application.h 69.76% <100.00%> (-15.95%) ⬇️
src/base/global.cpp 70.58% <100.00%> (-4.26%) ⬇️
src/kinetics/Reaction.cpp 81.07% <0.00%> (+0.17%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dba120...4a6c372. Read the comment docs.

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ischoegl! This looks like it will be a nice change, and it'll be nice to be able to manage warnings by built-in Python methods.

License.txt Show resolved Hide resolved
include/cantera/base/logger.h Show resolved Hide resolved
include/cantera/base/logger.h Outdated Show resolved Hide resolved
include/cantera/cython/wrappers.h Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

@bryanwweber ... thank you for the review!

I ended up implementing a more comprehensive C++ backend for a more fine-grained warning system that should mesh well with various API's. I also included a test that ensures that warnings are raised correctly.

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ischoegl! A couple more small things here.

include/cantera/base/global.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_utils.py Outdated Show resolved Hide resolved
@ischoegl ischoegl force-pushed the translate-warnings-to-Python branch 3 times, most recently from dc14d7f to e451173 Compare February 20, 2022 03:46
@ischoegl
Copy link
Member Author

ischoegl commented Feb 20, 2022

@bryanwweber ... 😅 Testing the warning system ended up creating battles with some of the work-arounds that were put in since 2.5.1 for testing purposes, specifically 5f5a005 and 80c1e56. As those methods do not allow for fine-grained control and are de-facto redundant, I decided to take them out.

Regarding your comment about pytest: using warning filters would allow for further removing methods that @speth put in earlier, specifically ct.make_deprecation_warnings_fatal() and ct.suppress_deprecation_warnings(). Removing those would be a lot more intrusive and should probably be handled in a different PR (if at all) ...

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ischoegl. Overall, I think this is a good change, and most of the implementation makes sense to me. There were just a few particular issues that I wanted to address.

License.txt Outdated Show resolved Hide resolved
include/cantera/base/global.h Outdated Show resolved Hide resolved
include/cantera/base/logger.h Show resolved Hide resolved
interfaces/cython/cantera/test/utilities.py Outdated Show resolved Hide resolved
src/base/application.h Show resolved Hide resolved
include/cantera/base/global.h Show resolved Hide resolved
@ischoegl ischoegl force-pushed the translate-warnings-to-Python branch 2 times, most recently from 9404138 to 2ac2059 Compare February 22, 2022 17:38
@ischoegl ischoegl requested a review from speth February 22, 2022 17:59
@ischoegl
Copy link
Member Author

@bryanwweber and @speth ... I believe both of your change requests are now fully addressed. Thanks for all suggestions!

speth
speth previously approved these changes Feb 27, 2022
@bryanwweber
Copy link
Member

This passes for me if I run pytest directly, but not running pytest via scons test-python. Will revert those changes if I can't find a quick fix 😢

@ischoegl
Copy link
Member Author

ischoegl commented Mar 3, 2022

@bryanwweber ... thanks for trying to simplify by introducing pytest.fixtures. While I agree that it's worthwhile testing this approach, it may be best to drop it from this PR. It's a tangent here and it can be easily tested in a dedicated PR?

@bryanwweber
Copy link
Member

@ischoegl I've got it working locally, I just need to commit & push

These functions are much easier to read than the previous wrapper
versions.
@bryanwweber
Copy link
Member

@ischoegl Presuming the checks pass this time, this should be good to :shipit:

@bryanwweber bryanwweber merged commit aa69bc2 into Cantera:main Mar 4, 2022
@ischoegl
Copy link
Member Author

ischoegl commented Mar 4, 2022

@bryanwweber ... 🎉 ... things passed. Looks like the fixture is pretty similar to what unittest did, which is good news! So thank you for looking into this! Beyond, I think all that's left is your approval 😉 ... :shipit:

@bryanwweber
Copy link
Member

@ischoegl Great minds!!! Merged 😄

@ischoegl ischoegl deleted the translate-warnings-to-Python branch March 5, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Channel C++ warnings to Python's warning system
3 participants