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
Improve warning messages in C++ #741
Conversation
08aca22
to
6da144a
Compare
Codecov Report
@@ Coverage Diff @@
## master #741 +/- ##
==========================================
- Coverage 70.84% 70.83% -0.01%
==========================================
Files 372 372
Lines 43704 43720 +16
==========================================
+ Hits 30962 30970 +8
- Misses 12742 12750 +8
Continue to review full report at Codecov.
|
f24db7d
to
5b7772f
Compare
5b7772f
to
6e60f2f
Compare
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 looks pretty good to me. I don't think it's a full fix for #683 yet, unless you also want to make the appropriate modifications to the Chemkin conversion scripts.
I would like to avoid adding Boost.Log to our dependencies -- While I generally like Boost, this is unfortunately one of parts that has a compiled library component, and my experience has been that these are very messy dependencies to deal with when you need to support multiple operating systems and a range of different Boost versions. However, a method for raising these warnings through the Python warnings
module would be interesting as a future extension.
@@ -40,41 +40,36 @@ Description: | |||
Chemical equilibrium. | |||
Equilibrium composition and pressure for a range of temperatures at constant density. | |||
|
|||
|
|||
**** WARNING **** | |||
UserWarning: 'NasaPoly2::validate': |
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 wondering if it would look better to format the warning message without quotes around the method name.
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.
No problem. Also suggesting CanteraWarning
to make things a little clearer
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.
👍 on CanteraWarning
@@ -55,7 +55,8 @@ int equil_example1(int job) | |||
std::cout << "Chemical equilibrium." << std::endl; | |||
if (job > 0) { | |||
std::cout << "Equilibrium composition and pressure for a " | |||
<< "range of temperatures at constant density." << std::endl; | |||
<< "range of temperatures at constant density." | |||
<< std::endl << std::endl; |
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.
It feels a little bit like cheating if you have to modify the output before a warning is issued to get it formatted as desired. One possibility to make multiline warnings more readable would be to automatically add a newline before and after a warning message if it already contains a newline.
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 added the extra newline here to remain consistent with the existing output, so this is pretty problem specific. I really don't like adding newlines before ... it's up to the user to format things as needed after.
@speth ... thanks for the review! Will make the changes which won’t take long.
I’ve been wondering about this also, but cannot think of an elegant solution. Definitely another PR |
One final comment for the record: I noticed that the following warnings depend on a log level. I don't want to change it without discussion as this PR should not change the overall behavior. cantera/src/transport/MMCollisionInt.cpp Lines 473 to 493 in 6e60f2f
|
3a7633a
to
f7cf50f
Compare
f7cf50f
to
a69151d
Compare
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.
With the change to ck2yaml
, I agree that this resolves #683.
Please fill in the issue number this pull request is fixing
Fixes #683
Changes proposed in this pull request
warn_user
in C++ layer (in analogy towarn_deprecated
)writelog
warnings withwarn_user
warn_user
for Troe when T2 (i.e. T**) is zeroNotes
Some
ck2cti
/ck2yaml
converters appear to create length-4 arrays for Troe parameters, which raises the warning that was suggested for #683 in the unit test suite. I.e.It's probably not worth fixing
ck2cti
, but it also affectsck2yaml
. Edit: the YAML import issue is fixed.Other thoughts
Long-term, it may make sense to use a logger that allows for a meaningful differentiation of levels, e.g. Boost.Log. The introduction of
warn_user
now will facilitate a central replacement inapplication.h/.cpp
in the future.