Change `SPGCONST` to `const` #258

LecrisUT · 2023-03-03T19:56:28Z

I think this is no longer necessary

codecov-commenter · 2023-03-03T20:02:21Z

Codecov Report

Patch coverage: 88.65% and project coverage change: +0.02 🎉

Comparison is base (3681a8a) 85.69% compared to head (493eeea) 85.72%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #258      +/-   ##
===========================================
+ Coverage    85.69%   85.72%   +0.02%     
===========================================
  Files           23       23              
  Lines         6069     6079      +10     
===========================================
+ Hits          5201     5211      +10     
  Misses         868      868

Flag	Coverage Δ
c_api	`73.68% <82.47%> (-0.03%)`	⬇️
fortran_api	`37.39% <55.67%> (+0.02%)`	⬆️
python_api	`82.87% <80.41%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
src/overlap.c	`93.20% <ø> (+0.23%)`	⬆️
src/site_symmetry.c	`97.95% <ø> (ø)`
src/spin.c	`91.88% <ø> (ø)`
src/spglib.c	`73.00% <70.58%> (+0.09%)`	⬆️
src/delaunay.c	`89.13% <75.00%> (ø)`
src/symmetry.c	`80.70% <83.33%> (ø)`
src/pointgroup.c	`76.94% <91.66%> (ø)`
src/mathfunc.c	`89.31% <92.00%> (-0.05%)`	⬇️
src/refinement.c	`84.85% <93.33%> (+0.02%)`	⬆️
src/cell.c	`79.48% <100.00%> (ø)`
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

atztogo · 2023-03-03T22:39:10Z

SPGCONST implies const, but const emits warning. I made it easy-to-read the code. Do you have a better idea?

LecrisUT · 2023-03-03T22:41:29Z

Oh, it is supposed to be the other way around, I'll test to see what warning it throws

atztogo · 2023-03-03T22:48:46Z

It is something like "incompatible pointer type".

I'm sorry for badly written codes. Spglib code was maintained almost alone for long time, so it was enough if I understand it. But now I should explain more about details.

LecrisUT · 2023-03-03T23:14:59Z

Ok, this one is an issue:
tmat here is marked as SPGCONST:

spglib/src/magnetic_spacegroup.c

Line 257 in 1d17e5e

Cell *msg_get_transformed_cell(const Cell *cell, SPGCONST double tmat[3][3],

But it call mat_inverse_matrix_d3 which changes it:

spglib/src/magnetic_spacegroup.c

Line 452 in 1d17e5e

mat_inverse_matrix_d3(tmat, (*ref_sg)->bravais_lattice, 0);

spglib/src/mathfunc.c

Line 268 in 1d17e5e

int mat_inverse_matrix_d3(double m[3][3], SPGCONST double a[3][3],

So it is indeed an implementation error that should be fixed. I.e. copy it if you want to edit it.

LecrisUT · 2023-03-03T23:24:58Z

spglib/src/mathfunc.c

Lines 268 to 289 in 1d17e5e

    
           int mat_inverse_matrix_d3(double m[3][3], SPGCONST double a[3][3], 
        
                                     const double precision) { 
        
               double det; 
        
               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); 
        
                   return 0; 
        
               } 
        
               c[0][0] = (a[1][1] * a[2][2] - a[1][2] * a[2][1]) / det; 
        
               c[1][0] = (a[1][2] * a[2][0] - a[1][0] * a[2][2]) / det; 
        
               c[2][0] = (a[1][0] * a[2][1] - a[1][1] * a[2][0]) / det; 
        
               c[0][1] = (a[2][1] * a[0][2] - a[2][2] * a[0][1]) / det; 
        
               c[1][1] = (a[2][2] * a[0][0] - a[2][0] * a[0][2]) / det; 
        
               c[2][1] = (a[2][0] * a[0][1] - a[2][1] * a[0][0]) / det; 
        
               c[0][2] = (a[0][1] * a[1][2] - a[0][2] * a[1][1]) / det; 
        
               c[1][2] = (a[0][2] * a[1][0] - a[0][0] * a[1][2]) / det; 
        
               c[2][2] = (a[0][0] * a[1][1] - a[0][1] * a[1][0]) / det; 
        
               mat_copy_matrix_d3(m, c); 
        
               return 1; 
        
           }

You don't need to save to a temporary array c here. The intent of m is always to be overwritten, and you don't use it anywhere else.

I think most of these issues can be resolved, it just needs a bit of working through to detangle the intents. Some doxygen comments would help, even if it is internal. Just what are each argument meant to be.

Oh, you're allowing for operations on the same pointer like mat_inverse_matrix_d3(a,a)

LecrisUT · 2023-03-04T01:00:15Z

Note that there are a lot of redundant func(const double variable). variable is always passed by value. const is only meant to be used for pointers and references to guarantee the function caller that it is not editted. When you pass by value, it is copies, so it becomes meaningless in that regard. Use func(double variable). Use something like CLion to automatically detect and correct these issues.

Anyway, there were only 4 spots that needed fixing, excluding the thousands of meaningless func(const double variable).

atztogo · 2023-03-04T02:06:49Z

Note that there are a lot of redundant func(const double variable). variable is always passed by value. const is only meant to be used for pointers and references to guarantee the function caller that it is not editted.

Indeed! I tend to use const to remember the variable not to be changed in the function. But you are correct. It should be fixed.

atztogo · 2023-03-04T02:11:52Z

Oh, you're allowing for operations on the same pointer like mat_inverse_matrix_d3(a,a)

This can happen. So I think it is safe to write in this way, but I don't mean this is the best way.

LecrisUT · 2023-03-04T06:17:16Z

Indeed. To be honest I don't know of an elegant way to do it without C++ templates, and otherwise if the non-copy is worth it for a 3x3 matrix. I would do it on my codes just for the designing exercise

LecrisUT · 2023-03-04T06:31:12Z

Rminder: if we merge #257 I need to add some words there.

With regards to versioning, I don't think it is that severe since there was an indication of the intent, we just might have allowed people to shoot themselves in the foot. I don't think we need to increase a major just for this. As long as the bindings are correctly configured also

Should be `const` Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT · 2023-04-06T14:14:33Z

We should merge this soon, the const is not enforced in the current state.

atztogo

Great. Thanks @LecrisUT!

LecrisUT mentioned this pull request Mar 3, 2023

Doxygen documentation #259

Draft

LecrisUT force-pushed the remove-spgconst branch from 7dcdd5c to 5882ecd Compare March 4, 2023 00:52

atztogo mentioned this pull request Mar 4, 2023

Remove const from non-pointer variables of function parameters #261

Open

LecrisUT added the enhancement Significant ehancements label Mar 4, 2023

LecrisUT force-pushed the remove-spgconst branch from 5882ecd to 6c0850a Compare March 6, 2023 15:28

LecrisUT added 2 commits April 5, 2023 09:32

Removed SPGCONST

2f3ce79

Should be `const` Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Fix const issues

493eeea

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT force-pushed the remove-spgconst branch from 6c0850a to 493eeea Compare April 5, 2023 07:32

LecrisUT requested review from atztogo and lan496 April 6, 2023 14:12

LecrisUT changed the title ~~Remove SPGCONST~~ Change SPGCONST to const Apr 6, 2023

atztogo approved these changes Apr 7, 2023

View reviewed changes

LecrisUT merged commit 77a8e5d into spglib:develop Apr 13, 2023

LecrisUT deleted the remove-spgconst branch June 8, 2023 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change `SPGCONST` to `const` #258

Change `SPGCONST` to `const` #258

Uh oh!

LecrisUT commented Mar 3, 2023

Uh oh!

codecov-commenter commented Mar 3, 2023 •

edited

Loading

Uh oh!

atztogo commented Mar 3, 2023

Uh oh!

LecrisUT commented Mar 3, 2023

Uh oh!

atztogo commented Mar 3, 2023

Uh oh!

LecrisUT commented Mar 3, 2023 •

edited

Loading

Uh oh!

LecrisUT commented Mar 3, 2023 •

edited

Loading

Uh oh!

LecrisUT commented Mar 4, 2023

Uh oh!

atztogo commented Mar 4, 2023

Uh oh!

atztogo commented Mar 4, 2023

Uh oh!

LecrisUT commented Mar 4, 2023

Uh oh!

LecrisUT commented Mar 4, 2023

Uh oh!

LecrisUT commented Apr 6, 2023

Uh oh!

atztogo left a comment

Uh oh!

Uh oh!

Change SPGCONST to const #258

Change SPGCONST to const #258

Uh oh!

Conversation

LecrisUT commented Mar 3, 2023

Uh oh!

codecov-commenter commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

atztogo commented Mar 3, 2023

Uh oh!

LecrisUT commented Mar 3, 2023

Uh oh!

atztogo commented Mar 3, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

LecrisUT commented Mar 4, 2023

Uh oh!

atztogo commented Mar 4, 2023

Uh oh!

atztogo commented Mar 4, 2023

Uh oh!

LecrisUT commented Mar 4, 2023

Uh oh!

LecrisUT commented Mar 4, 2023

Uh oh!

LecrisUT commented Apr 6, 2023

Uh oh!

atztogo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Change `SPGCONST` to `const` #258

Change `SPGCONST` to `const` #258

codecov-commenter commented Mar 3, 2023 •

edited

Loading

LecrisUT commented Mar 3, 2023 •

edited

Loading

LecrisUT commented Mar 3, 2023 •

edited

Loading