Skip to content

Increase max attempts for reduced cells#339

Merged
LecrisUT merged 9 commits intospglib:developfrom
atztogo:reduce-cell-max-attempt
Nov 17, 2023
Merged

Increase max attempts for reduced cells#339
LecrisUT merged 9 commits intospglib:developfrom
atztogo:reduce-cell-max-attempt

Conversation

@atztogo
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

LecrisUT commented Sep 28, 2023

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
Copy Markdown
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
Copy Markdown
Collaborator

LecrisUT commented Oct 11, 2023

C code, it should be dynamic

@LecrisUT
Copy link
Copy Markdown
Collaborator

LecrisUT commented Oct 11, 2023

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
Copy Markdown

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
Copy Markdown
Collaborator Author

atztogo commented Oct 11, 2023

Thanks @LecrisUT. I will add tests.

@LecrisUT
Copy link
Copy Markdown
Collaborator

LecrisUT commented Oct 11, 2023

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

LecrisUT commented Oct 28, 2023

I'll work on writing the tests today

@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
Copy Markdown
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
Copy Markdown
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
@lan496
Copy link
Copy Markdown
Member

lan496 commented Oct 31, 2023

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

@LecrisUT
Copy link
Copy Markdown
Collaborator

LecrisUT commented Oct 31, 2023

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