Created SPGLIB_INFO messaging level #461

atztogo · 2024-04-01T00:54:20Z

Currently spglib has two levels of messaging: debug_print and warning_print. This PR introduces another level info_print.

Most of warning_print were replaced by info_print.

c-function	c-macro	cmake-option	cmake-default
`debug_print`	`SPGDEBUG`	`SPGLIB_DEBUG`	OFF
`warning_print`	`SPGWARNING`	`SPGLIB_WARNING`	ON
`info_print`	`SPGINFO`	`SPGLIB_INFO`	OFF

Resolve #338

atztogo · 2024-04-01T05:49:36Z

@LecrisUT, this PR is conflicting with #457. This PR is to show that I consider what I have to do, but is not necessarily merged.

LecrisUT

I think we should use the IDE navigation and see how the functions are called. If they end up in a top-level err, if they are recovered, etc.

LecrisUT · 2024-04-01T07:38:25Z

src/magnetic_spacegroup.c

-        warning_print("  From DB matching: %d\n", msgtype.type);
+        info_print("Inconsistent MSG type:\n");
+        info_print("  From FSG and XSG: %d\n", type);
+        info_print("  From DB matching: %d\n", msgtype.type);
        goto err;


Is this an info or a warning? There are other goto err including ones with malloc issues.

This depends on how msg_identify_magnetic_space_group_type is meant to be used and if any issue is recoverable, expected, etc.

This is an unreachable situation without something floating-point error. So, I guess this will be info.

If it's unreachable like an assert it should be warning. Assuming the floating point error is not a dynamic tolerance

I will leave whether info or warning to you, but let me put more context.
If the parameters symprec and mag_symprec are sufficiently small, this is like the assert. But if these are too large, this assertion may fail. In that case, I recommend to decrease these parameter values.

Since these are tied to a user input, these would not be classified as assert (assert is only for debug if you are sure that users would not encounter it, that is why it is gated by NDEBUG), but as runtime try-except-like functionality.

It seems it could be either info or warning, but these are quite hard to detangle, so I am not sure how to classify either, since some are in a for loop, some are not. Ultimately we are mostly limited by the C language that we cannot use libraries like spdlog, which would make these issues obsolete (since we can dynamically enable/disable/redirect logging, including having python native logs)

Let's go with info.

LecrisUT · 2024-04-01T07:45:33Z

src/determination.c

-        warning_print("spglib: Attempt %d tolerance = %f failed.", attempt,
-                      tolerance);
-        warning_print(" (line %d, %s).\n", __LINE__, __FILE__);
+        info_print("spglib: Attempt %d tolerance = %f failed.", attempt,


For these tolerance, I will move them to debug and have only one info when it exits

LecrisUT · 2024-04-01T07:52:08Z

src/pointgroup.c

@@ -1121,8 +1121,8 @@ static void layer_check_and_sort_axes(int axes[3], int const aperiodic_axis) {
    } else {
        // Warning when rot_axes[axes[i]][aperiodic_axis] == -2, -3, 2, 3
        // I am not sure if this would ever happen.


Comment above seems to indicate it is a proper warning that at least we should know about if it happens

This code is introduced for layer group, but we will not want to expose the feature for users since we should reconsider the algorithm, and the implementation will be redesigned. This will be a work not only the code redesign but require some scientific discussion. The code lines for the layer group implementation were inserted in many places and the refactoring will need some effort and take time. So I just have been leaving it untouched.

LecrisUT · 2024-04-01T07:55:55Z

src/magnetic_spacegroup.c

@@ -908,9 +908,9 @@ static VecDBL *get_changed_pure_translations(double const tmat[3][3],

    /* Sanity check */
    if (count != size) {


This part is conflicting with the comment above

This function transforms the basis but when the distortion of the input crystal structure is large and with naive choice of the tolerance, this fails. count == size is a minimum check that should satisfy the condition. So the comment says Sanity check. Could you recommend a better word?

LecrisUT · 2024-04-01T07:59:21Z

src/mathfunc.c

@@ -266,7 +266,7 @@ int mat_inverse_matrix_d3(double m[3][3], double const a[3][3],
    double c[3][3];
    det = mat_get_determinant_d3(a);
    if (mat_Dabs(det) < precision) {
-        warning_print("spglib: No inverse matrix (det=%f)\n", det);
+        info_print("spglib: No inverse matrix (det=%f)\n", det);


Maybe this is a debug_print?

So if I see this message, I will see that the crystal structure is distorted. But as you commented, it is not possible to see it from the local piece of the code.

lan496 · 2024-05-12T02:41:22Z

Close this PR because #457 was merged

Created SPGLIB_INFO messaging level

17f93d0

atztogo requested a review from LecrisUT April 1, 2024 04:38

LecrisUT reviewed Apr 1, 2024

View reviewed changes

lan496 mentioned this pull request Apr 2, 2024

fix: Debug and warning messages #457

Merged

7 tasks

lan496 closed this May 12, 2024

		@@ -908,9 +908,9 @@ static VecDBL *get_changed_pure_translations(double const tmat[3][3],

		/* Sanity check */
		if (count != size) {

Created SPGLIB_INFO messaging level #461

Created SPGLIB_INFO messaging level #461

Uh oh!

Conversation

atztogo commented Apr 1, 2024

Uh oh!

atztogo commented Apr 1, 2024

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lan496 Apr 1, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lan496 commented May 12, 2024

Uh oh!

Uh oh!

lan496 Apr 1, 2024 •

edited

Loading