Skip to content

Minor simplification of trim_cell#347

Merged
atztogo merged 2 commits intospglib:developfrom
atztogo:trim-with-tmat
Oct 18, 2023
Merged

Minor simplification of trim_cell#347
atztogo merged 2 commits intospglib:developfrom
atztogo:trim-with-tmat

Conversation

@atztogo
Copy link
Copy Markdown
Collaborator

@atztogo atztogo commented Oct 17, 2023 •
edited
Loading

The nearest integer matrix of axis_inv is tmat_p_i at line ~520 in function translate_atoms_in_trimmed_lattice. I prefer the later because I want to have a transformation matrix to be an integer matrix or inverse of an integer matrix whenever possible.

Actually, axis_inv is unnecessary to be computed in translate_atoms_in_trimmed_lattice because it is same as tmp_mat in function trim_cell that calls translate_atoms_in_trimmed_lattice and translate_atoms_in_trimmed_lattice is called only from trim_cell.

Although axis_inv and tmat_p_i can be slightly different if the input cell is distorted, I would like to suggest this change.

@atztogo atztogo requested a review from lan496 October 17, 2023 09:40
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 17, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (a96d72a) 83.56% compared to head (c7de271) 83.55%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #347      +/-   ##
===========================================
- Coverage    83.56%   83.55%   -0.01%     
===========================================
  Files           24       24              
  Lines         8157     8154       -3     
===========================================
- Hits          6816     6813       -3     
  Misses        1341     1341
Flag Coverage Δ
c_api 76.43% <100.00%> (-0.01%) ⬇️
fortran_api 56.29% <100.00%> (-0.02%) ⬇️
python_api 80.42% <100.00%> (-0.01%) ⬇️
unit_tests 1.24% <0.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
src/cell.c 74.56% <100.00%> (-0.27%) ⬇️

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

@lan496
Copy link
Copy Markdown
Member

lan496 commented Oct 17, 2023 •
edited
Loading

I also agree to use integer matrices as possible. BTW, can you write a description for Cell *cel_trim_cell(...)? After reading, I understand that this function computes a mapping of atom positions in a small cell trimmed_lattice. I feel this is a bit far from its function name.

@LecrisUT LecrisUT self-requested a review October 17, 2023 12:54
@atztogo
Copy link
Copy Markdown
Collaborator Author

atztogo commented Oct 17, 2023

@lan496, thanks for your review. I added some descriptions.

BTW, can you write a description for Cell *cel_trim_cell(...)? After reading, I understand that this function computes a mapping of atom positions in a small cell trimmed_lattice. I feel this is a bit far from its function name.

Do you mean the word trim is not intuitive?

atztogo
atztogo commented Oct 17, 2023
View reviewed changes
src/cell.c
Comment on lines +64 to +65
// @param tensor_rank rank of site tensors for magnetic symmetry. Set -1 if
// not used.
Copy link
Copy Markdown
Collaborator Author

@atztogo atztogo Oct 17, 2023

Choose a reason for hiding this comment

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

My line length setting is considered different from the original code. Do we have a formatter to make it same?

Copy link
Copy Markdown
Collaborator

@LecrisUT LecrisUT Oct 17, 2023

Choose a reason for hiding this comment

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

Are you using clang-format by default? I'm not sure on VSCode, but on CLion there is an option to use clang-format instead of the built-in

Copy link
Copy Markdown
Collaborator Author

@atztogo atztogo Oct 17, 2023

Choose a reason for hiding this comment

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

Thanks for your info. @LecrisUT. I use VSCode, and my current setting is like below,

    "C_Cpp.clang_format_style": "{BasedOnStyle: Google, IndentWidth: 4}",

I found some information, https://code.visualstudio.com/docs/cpp/cpp-ide#_code-formatting . I should read this in detail.

@lan496
Copy link
Copy Markdown
Member

lan496 commented Oct 17, 2023

Do you mean the word trim is not intuitive?

Yes, I felt nonintuitive because Cell *cel_trim_cell already gets trimmed_lattice (so called "trimmed lattice") as an input.

Cell *cel_trim_cell(int *mapping_table, const double trimmed_lattice[3][3],
                    const Cell *cell, const double symprec)

@lan496
Copy link
Copy Markdown
Member

lan496 commented Oct 17, 2023

Sorry, I missed the returned trim_cell for this function. So, the function name should be fine.

@atztogo atztogo merged commit bf0b4b7 into spglib:develop Oct 18, 2023
@atztogo atztogo deleted the trim-with-tmat branch October 18, 2023 02:13
@atztogo
Copy link
Copy Markdown
Collaborator Author

atztogo commented Oct 18, 2023

Clearly the code is confusing and we should refactor it, but not in this PR.

@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

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@atztogo @codecov-commenter @lan496 @LecrisUT