fix: Debug and warning messages #457
Conversation
3a8f6f3 to
c4e959c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #457 +/- ##
===========================================
+ Coverage 83.59% 83.80% +0.21%
===========================================
Files 24 25 +1
Lines 8118 8164 +46
Branches 1693 1700 +7
===========================================
+ Hits 6786 6842 +56
+ Misses 1332 1322 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dad97d4 to
5291093
Compare
src/pointgroup.c
Outdated
| @@ -401,8 +401,7 @@ Pointgroup ptg_get_transformation_matrix(int transform_mat[3][3], | |||
| get_axes(axes, pointgroup.laue, &pointsym, aperiodic_axis); | |||
| set_transformation_matrix(transform_mat, axes); | |||
| } else { | |||
| warning_print("spglib: No point group was found "); | |||
| warning_print("(line %d, %s).\n", __LINE__, __FILE__); | |||
| warning_print("spglib: No point group was found\n"); | |||
There was a problem hiding this comment.
This one is a good example of the things that need to be reviewed:
From #338 (comment), it seems that this message is one of those that is not related to an error. Thus in a normal run, this message gets spammed a lot. Do you think it should be changed to a debug message?
There was a problem hiding this comment.
Fwiw, in my opinion this message should be changed to a debug message, that ideally is only printed if the overall routine fails or the code is being run in some debug type mode. Thanks for looking into this @LecrisUT!
Safe to use these days: https://en.wikipedia.org/wiki/Pragma_once Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Use `inline` to simplify debug message definitions Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
- Debug messages are disabled by default unless `SPGLIB_DEBUG` is defined - Warning messages are enabled by default unless `SPGLIB_WARNING=OFF` Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
- Make sure messages have newline - Convert any warnings inside an attempt to `debug_print` - Remove the `__LINE__`/`__FILE__` outputs. Request reproducable input and use a proper debugger Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
See previous commit Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
|
I took a more conservative approach in b7adcc0. It is unfortunate that some messages appear both in a lopp and in the main |
| if (warning_env == NULL) return true; | ||
| // Check if SPGLIB_WARNING is disabled. Not performing case-insensitive | ||
| // checks. | ||
| if (strcmp(warning_env, "OFF") == 0) return false; |
There was a problem hiding this comment.
Why only warning_enabled accepts "OFF" compared to debug_enabled and info_enabled?
There was a problem hiding this comment.
The others are default: false, while this is the only one to be default: true. For default false, we can assume if you set an environment variable the intent is to enable it. For the opposite case it is more ambiguous, that's why the explicit check of the value.
| @@ -348,7 +343,7 @@ static int step2(NiggliParams *p) { | |||
| static int step2_for_layer(NiggliParams *p) { | |||
| if (p->B > p->C + p->eps || (!(fabs(p->B - p->C) > p->eps) && | |||
| fabs(p->eta) > fabs(p->zeta) + p->eps)) { | |||
| warning_print( | |||
| debug_print( | |||
I have cherry-picked only select changes from there, because I am unsure about converting all I have also added another item on the list "Add |
|
Thanks for your work on this! I tried and these can't be captured with |
|
You can set |
|
Ok great, thanks very much for this! One last question if you don't mind, is there any way to ensure that when users upgrade |
|
That I am still thinking about in #462. In the meantime, you can try: $ pip install spglib --config-settings=cmake.define.SPGLIB_SHARED_LIBS=OFFI am not sure if it works with Other options would be to |
|
Thanks @LecrisUT! |
SPGLIB_DEBUGandSPGLIB_WARNINGScmake variables are for controlling if these messages are compiled at all or notstdoutfor debug messages andstderrfor warningattemptwarning_print/info_printat the end offor (attempt)loopsCloses #338, Closes #336
Depends-on: #431