Skip to content

Increase max attempts for reduced cells #339

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Nov 17, 2023
Merged

Increase max attempts for reduced cells #339

merged 9 commits into from
Nov 17, 2023

Conversation

atztogo
Copy link
Collaborator

@atztogo atztogo commented Sep 28, 2023

Currently max number of iteration to attempt Niggli and Delaunay cells are 100. I want to increase the number because this number is not enough for highly oblique choice of basis vectors under the same lattice.

This PR suggests increasing the number from 100 to 1000.

This change may As far as I ran simply pytest and compared the run time, I didn't find noticeable difference.

@atztogo atztogo requested a review from lan496 September 28, 2023 13:28
@LecrisUT
Copy link
Collaborator

Let's expose this via environment variable. I'll add a review with the interface

@LecrisUT LecrisUT self-requested a review September 28, 2023 13:33
@atztogo
Copy link
Collaborator Author

atztogo commented Oct 11, 2023

Let's expose this via environment variable. I'll add a review with the interface

Is this to be done in C code or in cmake?

@LecrisUT
Copy link
Collaborator

C code, it should be dynamic

@LecrisUT
Copy link
Collaborator

I've added an implementation to get the NUM_ATTEMPTS from the environment variable. I did not add any tests for this yet.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (8fab0b0) 83.68% compared to head (74cc0e7) 83.80%.

Files Patch % Lines
src/delaunay.c 93.33% 2 Missing ⚠️
src/niggli.c 90.90% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #339      +/-   ##
===========================================
+ Coverage    83.68%   83.80%   +0.11%     
===========================================
  Files           24       24              
  Lines         8144     8167      +23     
===========================================
+ Hits          6815     6844      +29     
+ Misses        1329     1323       -6
Flag Coverage Δ
c_api 77.18% <92.68%> (+0.72%) ⬆️
fortran_api 56.19% <21.95%> (-0.11%) ⬇️
python_api 80.47% <64.51%> (-0.08%) ⬇️
unit_tests 1.24% <0.00%> (-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.

@atztogo
Copy link
Collaborator Author

atztogo commented Oct 11, 2023

Thanks @LecrisUT. I will add tests.

@LecrisUT
Copy link
Collaborator

Thanks @LecrisUT. I will add tests.

I think this one might be a bit tricky if you are not used to making test fixtures. If you need more help, ping me next week and I'll try to add that:

@atztogo
Copy link
Collaborator Author

atztogo commented Oct 12, 2023

Thanks @LecrisUT, I need your help if it can be done quickly by you. As examples, I added simple tests (ca06dcf) without using setenv. These tests pass with default SPGLIB_NUM_ATTEMPTS = 1000, but should fail with 100.

@atztogo atztogo mentioned this pull request Oct 28, 2023
Closed
2 tasks
@atztogo atztogo self-assigned this Oct 28, 2023
lan496
lan496 reviewed Oct 28, 2023
View reviewed changes
lan496
lan496 approved these changes Oct 28, 2023
View reviewed changes
Copy link
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply. I have left an optional comment

@LecrisUT
Copy link
Collaborator

I'll work on writing the tests today

atztogo and others added 6 commits October 29, 2023 19:30
@atztogo @LecrisUT
Increase max attempts for reduced cells
16f5e25
@LecrisUT
Add environment variable SPGLIB_NUM_ATTEMPTS
6702c53 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@atztogo @LecrisUT
Add tests for niggli and delaunay reductions for default SPGLIB_NUM_A…
0a879de 
…TTEMPTS=1000
@LecrisUT
Include SPGLIB_NUM_ATTEMPTS environment test
2df67f9 
Signed-off-by: Cristian Le <git@lecris.dev>
@LecrisUT
Add MSVC wrapper for setenv
5d6bae1 
Signed-off-by: Cristian Le <git@lecris.dev>
@LecrisUT
Call get_num_attempts() only once
e2c8d05 
Signed-off-by: Cristian Le <git@lecris.dev>
@LecrisUT LecrisUT added bug enhancement Significant ehancements labels Oct 30, 2023
@LecrisUT LecrisUT requested a review from lan496 October 30, 2023 09:20
LecrisUT
LecrisUT approved these changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@LecrisUT LecrisUT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle it would be nice to move get_num_attempts (as well as other get_debug and get_warning) to a global call, but I am putting off that implementation for now.

lan496
lan496 requested changes Oct 30, 2023
View reviewed changes
Copy link
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setenv looks nice for testing max attempts. I've left similar suggestions as I commented.

atztogo and others added 2 commits October 30, 2023 19:00
@atztogo @lan496
Update src/delaunay.c
ff21ad5 
Co-authored-by: Kohei Shinohara <kohei19950508@gmail.com>
@LecrisUT
Minor changes to the variable initialization
69885bd 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@lan496
Copy link
Member

lan496 commented Oct 31, 2023

@atztogo @LecrisUT LGTM. Can I merge this PR?

@LecrisUT
Copy link
Collaborator

I forgot to add a test for spglib_error. Let me add that real quick and then it's 👍 from me

LecrisUT
LecrisUT reviewed Oct 31, 2023
View reviewed changes
@LecrisUT LecrisUT enabled auto-merge November 17, 2023 09:49
@LecrisUT LecrisUT merged commit b02dedb into spglib:develop Nov 17, 2023
@atztogo atztogo deleted the reduce-cell-max-attempt branch November 17, 2023 12:08
@Porkepix Porkepix mentioned this pull request Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LecrisUT LecrisUT LecrisUT approved these changes

@lan496 lan496 lan496 approved these changes

Assignees

@atztogo atztogo

@lan496 lan496

Labels
bug enhancement Significant ehancements
Projects
Spglib developers' kanban
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@atztogo @LecrisUT @codecov-commenter @lan496