fix: Debug and warning messages by LecrisUT · Pull Request #457 · spglib/spglib

LecrisUT · 2024-03-27T12:20:09Z

Refactor message implementation. Avoid macros whenever possible
Enable/Disable messages by environment variable. SPGLIB_DEBUG and SPGLIB_WARNINGS cmake variables are for controlling if these messages are compiled at all or not
Write appropriately to stdout for debug messages and stderr for warning
Review what messages are warnings and which ones are debug messages
- Check that warning messages are not associated with attempt
- Check that warning messages are associated with a failure/error
- Add warning_print/info_print at the end of for (attempt) loops

Closes #338, Closes #336

Depends-on: #431

codecov · 2024-03-27T15:37:13Z

Codecov Report

Attention: Patch coverage is 20.52980% with 120 lines in your changes are missing coverage. Please review.

Project coverage is 83.80%. Comparing base (2edad27) to head (b7adcc0).

Files	Patch %	Lines
src/spglib.c	0.00%	23 Missing ⚠️
src/refinement.c	5.88%	16 Missing ⚠️
src/symmetry.c	6.25%	15 Missing ⚠️
src/debug.c	54.83%	14 Missing ⚠️
src/primitive.c	0.00%	9 Missing ⚠️
src/cell.c	0.00%	8 Missing ⚠️
src/delaunay.c	0.00%	6 Missing ⚠️
src/mathfunc.c	0.00%	6 Missing ⚠️
src/kpoint.c	0.00%	5 Missing ⚠️
src/overlap.c	0.00%	5 Missing ⚠️
... and 6 more

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

Flag	Coverage Δ
c_api	`74.71% <17.88%> (+0.27%)`	⬆️
fortran_api	`56.20% <3.97%> (+0.04%)`	⬆️
python_api	`80.04% <15.43%> (+0.34%)`	⬆️
unit_tests	`13.48% <3.97%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

LecrisUT · 2024-03-27T18:15:27Z

src/pointgroup.c

    } 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");


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?

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 <[email protected]>

Use `inline` to simplify debug message definitions Signed-off-by: Cristian Le <[email protected]>

- 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 <[email protected]>

Signed-off-by: Cristian Le <[email protected]>

- 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 <[email protected]>

See previous commit Signed-off-by: Cristian Le <[email protected]>

Signed-off-by: Cristian Le <[email protected]>

LecrisUT · 2024-04-01T13:39:28Z

I took a more conservative approach in b7adcc0. It is unfortunate that some messages appear both in a lopp and in the main spglib.h interface so it's not a clear cut how to categorize them. I still think I got these messages mislabeled. We could go even more conservative and only address the messages that were reported by users, but that would take some time and bug reports to get it right.

lan496 · 2024-04-02T10:55:36Z

src/debug.c

+    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;


Why only warning_enabled accepts "OFF" compared to debug_enabled and info_enabled?

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.

lan496 · 2024-04-02T11:08:22Z

src/niggli.c

    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(


It looks like warning

lan496

I assume the fixes in #461 will be included in this PR.

LecrisUT · 2024-04-02T11:18:07Z

I assume the fixes in #461 will be included in this PR.

I have cherry-picked only select changes from there, because I am unsure about converting all warning_print to info_print. If you find some other that need porting, feel free to mention the file-line.

I have also added another item on the list "Add warning_print/info_print at the end of for (attempt) loops". Could use some help with those.

lan496

I'm not so opinionated in info or warning and the refactoring looks good to me.

kavanase · 2024-04-13T17:24:48Z

Thanks for your work on this!
Just to flag @LecrisUT in case it flies under the radar, with spglib 2.4.0 and updating DYLD_LIBRARY_PATH (so spglib.spg_get_version_full() shows the correct new version), most of the verbose warning messages are no longer printed (🙌 ), but these two are still printed:

spglib: ssm_get_exact_positions failed.
spglib: get_bravais_exact_positions_and_lattice failed.

I tried and these can't be captured with warnings or StringIO. They disappear when the same code is run again.

LecrisUT · 2024-04-13T17:47:11Z

You can set SPGLIB_WARNING=OFF as an environment variable. It should work within Python as well as long as a clean shell environment is not created every time

kavanase · 2024-04-13T18:07:27Z

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 spglib, that it uses the correct DYLD_LIBRARY_PATH (without having to manually set it as I did with export DYLD_LIBRARY_PATH=/path/to/site-packages/spglib/lib:$DYLD_LIBRARY_PATH)?

LecrisUT · 2024-04-13T18:16:30Z

That I am still thinking about in #462. In the meantime, you can try:

$ pip install spglib --config-settings=cmake.define.SPGLIB_SHARED_LIBS=OFF

I am not sure if it works with pip install spglib, but it should work with pip install git+https://github.com/spglib/spglib. This one links the _spglib library statically to the bundled version

Other options would be to patchelf --add-rpath=$ORIGIN/libs (not sure if I got the path correct or if $ORIGIN needs to be escaped) to the _spglib library. RPATH should have priority when linking, and adding this would be like specifying DYLD_LIBRARY_PATH statically. patchelf should be available for MacOS in homebrew 🤷

kavanase · 2024-04-13T18:44:59Z

Thanks @LecrisUT!
Just to confirm, pip install git+https://github.com/spglib/spglib --config-settings=cmake.define.SPGLIB_SHARED_LIBS=OFF works for this, but pip install spglib --config-settings=cmake.define.SPGLIB_SHARED_LIBS=OFF doesn't

This was referenced Mar 27, 2024

Most of warning_print should should not be displayed. #338

Closed

spglib-2.1.0 shows warnings when using get_spacegroup method but spglib-2.0.2 does not #336

Closed

LecrisUT force-pushed the fix/messages branch from 1fca0f5 to 57509fa Compare March 27, 2024 13:34

LecrisUT mentioned this pull request Mar 27, 2024

ci: Test CMake version #431

Merged

LecrisUT added this to the 2.4 milestone Mar 27, 2024

LecrisUT mentioned this pull request Mar 27, 2024

Release v2.4.0 #434

Closed

5 tasks

LecrisUT force-pushed the fix/messages branch 3 times, most recently from 3a8f6f3 to c4e959c Compare March 27, 2024 15:29

LecrisUT requested review from atztogo and lan496 March 27, 2024 15:33

LecrisUT assigned lan496 Mar 27, 2024

LecrisUT force-pushed the fix/messages branch 2 times, most recently from dad97d4 to 5291093 Compare March 27, 2024 17:43

LecrisUT commented Mar 27, 2024

View reviewed changes

atztogo mentioned this pull request Apr 1, 2024

Created SPGLIB_INFO messaging level #461

Closed

LecrisUT and others added 10 commits April 1, 2024 13:21

Use pragma once

ed78f43

Safe to use these days: https://en.wikipedia.org/wiki/Pragma_once Signed-off-by: Cristian Le <[email protected]>

Refactor debug.c

03d87be

Use `inline` to simplify debug message definitions Signed-off-by: Cristian Le <[email protected]>

Control messages by environment variable

223f0af

- 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 <[email protected]>

Print accordingly to stdout/stderr

117ca7b

Signed-off-by: Cristian Le <[email protected]>

Use helper warning_memory

f5c0345

Signed-off-by: Cristian Le <[email protected]>

Review warning print usage

5667adf

- 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 <[email protected]>

Review debug print usage

a79a78c

See previous commit Signed-off-by: Cristian Le <[email protected]>

Document changes

80f31ae

Signed-off-by: Cristian Le <[email protected]>

Add info message parsing

db0a1a1

Signed-off-by: Cristian Le <[email protected]>

Demote a few warning messages

b7adcc0

Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the fix/messages branch from 5291093 to b7adcc0 Compare April 1, 2024 13:30

lan496 reviewed Apr 2, 2024

View reviewed changes

kavanase mentioned this pull request Apr 2, 2024

Bump the python-packages group with 1 update SMTG-Bham/doped#59

Closed

lan496 approved these changes Apr 10, 2024

View reviewed changes

atztogo approved these changes Apr 10, 2024

View reviewed changes

lan496 merged commit 6e20e03 into spglib:develop Apr 10, 2024

LecrisUT deleted the fix/messages branch April 10, 2024 10:55

kavanase mentioned this pull request May 23, 2024

Feature request: Support versions of spglib above 2.0.2 SMTG-Bham/doped#73

Closed

Conversation

LecrisUT commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LecrisUT Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

kavanase Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

LecrisUT commented Apr 1, 2024

Uh oh!

lan496 Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

LecrisUT Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

lan496 Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

lan496 left a comment

Choose a reason for hiding this comment

Uh oh!

LecrisUT commented Apr 2, 2024

Uh oh!

lan496 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kavanase commented Apr 13, 2024

Uh oh!

LecrisUT commented Apr 13, 2024

Uh oh!

kavanase commented Apr 13, 2024

Uh oh!

LecrisUT commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kavanase commented Apr 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LecrisUT commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

lan496 left a comment •

edited

Loading

LecrisUT commented Apr 13, 2024 •

edited

Loading