Skip to content
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.

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

Update examples in C and Fortran #242

Merged
merged 24 commits into from Mar 2, 2023
Merged

Update examples in C and Fortran #242

merged 24 commits into from Mar 2, 2023

Conversation

atztogo
Copy link
Collaborator

@atztogo atztogo commented Feb 22, 2023

PR for #236.

  • simplify examples example_c and example_f
  • provide a conceptual example in the readme of what spglib does

The examples in C seem not so bad. So I just changed the function names of test_* to example_*.
The Fortran code of examples in Fortran was very dirty. I made it a little bit cleaner.

Conceptual examples for spg_get_dataset should be written in README.md.

Closes: #236

example/example.c Show resolved Hide resolved
example/example_f08.f90 Outdated Show resolved Hide resolved
@LecrisUT LecrisUT added this to the 2.1 milestone Feb 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (bbb4f1e) 85.69% compared to head (86566c4) 85.69%.

📣 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     #242   +/-   ##
========================================
  Coverage    85.69%   85.69%           
========================================
  Files           23       23           
  Lines         6069     6069           
========================================
  Hits          5201     5201           
  Misses         868      868           
Flag Coverage Δ
c_api 73.70% <ø> (ø)
fortran_api 37.37% <ø> (ø)
python_api 82.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 23, 2023

Since users in C and Fortran may be exclusive, I attempted to change directory structure and the names.

c-example/   # renamed from example/
fortran/example/  # new directory

Dependency of CMakeLists.txt files should be broken though I don't check it. Since I am afraid of conflict to modify that in upper directories, I just let it mostly unchanged.

@atztogo atztogo changed the title [WIP] Update examples in C and Fortran Update examples in C and Fortran Feb 23, 2023
@atztogo
Copy link
Collaborator Author

atztogo commented Feb 23, 2023

It is done except for CMakeLists.txt files.

@LecrisUT
Copy link
Collaborator

Since users in C and Fortran may be exclusive, I attempted to change directory structure and the names.

c-example/   # renamed from example/
fortran/example/  # new directory

I agree with that, but can it be structured like:

|-- example
  |-- c_api
  |-- fortran_api
  |-- python_api

Dependency of CMakeLists.txt files should be broken though I don't check it. Since I am afraid of conflict to modify that in upper directories, I just let it mostly unchanged.

I'll look into that. Just re-request a review to ping me when you want me to look into it.

c-example/README.md Outdated Show resolved Hide resolved
c-example/README.md Outdated Show resolved Hide resolved
c-example/README.md Outdated Show resolved Hide resolved
c-example/README.md Outdated Show resolved Hide resolved
c-example/example.c Outdated Show resolved Hide resolved
fortran/example/README.md Outdated Show resolved Hide resolved
fortran/example/README.md Outdated Show resolved Hide resolved
fortran/example/README.md Outdated Show resolved Hide resolved
fortran/example/README.md Outdated Show resolved Hide resolved
c-example/example.c Outdated Show resolved Hide resolved
c-example/example.c Outdated Show resolved Hide resolved
c-example/example.c Outdated Show resolved Hide resolved
c-example/example.c Outdated Show resolved Hide resolved
c-example/example.c Outdated Show resolved Hide resolved
fortran/README.md Outdated Show resolved Hide resolved
fortran/example/README.md Outdated Show resolved Hide resolved
@LecrisUT
Copy link
Collaborator

Almost done. One final thing though, why not the following folder structure:

|-- example
  |-- c_api
  |-- fortran_api
  |-- python_api

(/example/c_api, etc.)

Top example readme tells about what common minimal example is implemented in all 3 and that a complete one is also provided.

@LecrisUT
Copy link
Collaborator

Ok, my final note, fortran files should be F90 instead of f90. It's related to how they read macros and everything. It's a good standard to use.

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 25, 2023

CI fails because CMakeLists.txts are not updated properly due to the change of directory structure and maybe f90 to F90. I think this is better treated in another PR after merging this PR and expect @LecrisUT's help.

@LecrisUT
Copy link
Collaborator

Please rebase onto develop. We cannot merge because of merge conflict.

$ git rebase origin/develop
$ git status
Check which files are conflicting and how to resolve it
$ emacs example/c_api/README.md
Command may be different because file was moved
Resolve conflicts by editting the blocks:
<<<<< HEAD
Content from origin/develop
=====
Content from branch examples
>>>>>
$ git rebase --continue

I can also recommend gitkraken if you don't feel comfortable with the git cli for this.

I will add commit fixes on top of this PR before merging when you feel it is ready.

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 25, 2023

Please rebase onto develop. We cannot merge because of merge conflict.

Thanks for your detailed instruction. No conflict was found in rebase.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Feb 25, 2023

Great. Pre-commit seems to fail

Oh, the merge is reporting changes from develop. A rebase would have made it cleaner to review.

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 25, 2023

Oh, the merge is reporting changes from develop. A rebase would have made it cleaner to review.

How can I do?

@LecrisUT
Copy link
Collaborator

LecrisUT commented Feb 25, 2023

Oh, the merge is reporting changes from develop. A rebase would have made it cleaner to review.

How can I do?

At this point will have to interactively rebase to drop the merge commit. But basically running git rebase instead of git merge.

I'll handle that later

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 25, 2023

At this point will have to interactively rebase to drop the merge commit. But basically running git rebase instead of git merge.

I made git rebase but when I wanted to push, git said I need to pull, and I did it with the merge way. Probably my last choice was wrong. What could I do in this case? I should force-push?

@LecrisUT
Copy link
Collaborator

https://github.com/atztogo/spglib/blob/4d66a9c249bbc96154585a01372ac8e82e41fa96/fortran/CMakeLists.txt#L2

Edit the file extension in the cmake files also.

I made git rebase but when I wanted to push, git said I need to pull, and I did it with the merge way. Probably my last choice was wrong. What could I do in this case? I should force-push?

Yes, you can freely force-push in these situations. Just never force-push the develop branch.

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 25, 2023

Edit the file extension in the cmake files also.

Yes, I made it. e2a01d5.

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 26, 2023

Is there anything left to do?

@LecrisUT
Copy link
Collaborator

Just to fix git history and cmake. I'll do that tomorrow

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 26, 2023

Just to fix git history and cmake. I'll do that tomorrow

It is clearly difficult for me to do. Thanks @LecrisUT.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
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.

@atztogo I have refactored your branch and fixed the examples. I have noticed that the python example is not equivalent to the others. Could you make some changes there, and if possible add some words to example/README.md?

You can work on the same branch again by:

$ git fetch
$ git reset --hard origin/examples

Otherwise we can open a new PR for that.

And of course, thanks for all the work on this 😄

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.

Thank you fully 👍 from me. Do you want to wait for @lan496 to review as well?

@atztogo
Copy link
Collaborator Author

atztogo commented Mar 1, 2023

Could you make some changes there, and if possible add some words to example/README.md?

Thank you for pointing it out. I did it at ddcc153 and 86566c4.

@lan496
Copy link
Member

lan496 commented Mar 2, 2023

I have no objection. Please merge if you are ready.

@atztogo atztogo merged commit 1d17e5e into spglib:develop Mar 2, 2023
@atztogo atztogo deleted the examples branch March 2, 2023 06:22
@atztogo
Copy link
Collaborator Author

atztogo commented Mar 2, 2023

Thanks @LecrisUT and @lan496!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify examples
4 participants