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

Add fortran interface test #230

Merged
merged 3 commits into from Feb 5, 2023
Merged

Conversation

atztogo
Copy link
Collaborator

@atztogo atztogo commented Feb 4, 2023

Initial fortran test was written following boilerplate of test_dummy.F90.

  • test_dummy.F90 was changed to test_spg_get_symmetry.f90.
  • c_interface_module.F90 was removed. Probably this will not be used.
  • test/fortran/utils.f90 contains utilities for fortran tests. This was written by @atztogo.
  • pre-commit modified file tails of CMakeLists.txt's.

Initial fortran test was written following boilerplate of test_dummmy.F90.

- test_dummy.F90 was changed to test_spg_get_symmetry.f90.
- c_interface_module.F90 was removed. Probably this will not be used.
- test/fortran/utils.f90 contains utilities for fortran tests.
- pre-commit modified file tails of CMakeLists.txt's.
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2023

Codecov Report

Base: 93.34% // Head: 93.34% // No change to project coverage 👍

Coverage data is based on head (484aed9) compared to base (2c6a42d).
Patch has no changes to coverable lines.

📣 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     #230   +/-   ##
========================================
  Coverage    93.34%   93.34%           
========================================
  Files           15       15           
  Lines          902      902           
========================================
  Hits           842      842           
  Misses          60       60           

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.

if (val /= ref_val) then
print '()'
print '(i0, "/=", i0)', val, ref_val
error stop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does error give you a traceback or equivalent? Would be difficult to debug without that. Hopefully cmake's boilerplate can save us

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume, if the test in C (and python) works, spglib in C is fine. Because the fortran interface is very simple wrapper, if the test in fortran went wrong, most likely the test code is wrong, but the fortran wrapper is probably not wrong. Therefore, this print information is used for debugging the test code.

To see the traceback, we may have to compile spgilb with debugging option. Can we set set(CMAKE_BUILD_TYPE Debug) when spglib_WITH_TESTS=on? Or, is it already so?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tested this a bit and even with Debug flag we don't get a traceback. At the same time though, there are no clean Fortran methods of doing so close to what standard C++, catch2 or GoogleTest (closest I've found is fortuno), but that doesn't define macros to automatically throw messages either.

But for what it's worth, I say it's good enough since we only need it to detect issues and then we can use a debugger.

Can we set set(CMAKE_BUILD_TYPE Debug) when spglib_WITH_TESTS=on

We shouldn't because users would want to run tests as well.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Feb 4, 2023

Also maybe worth changing the names to test_fortran_ to not name clash with C ones

@atztogo
Copy link
Collaborator Author

atztogo commented Feb 4, 2023

Also maybe worth changing the names to test_fortran_ to not name clash with C ones

Absolutely!

@atztogo atztogo merged commit 1b1729c into spglib:develop Feb 5, 2023
@atztogo atztogo deleted the initial-fortran-test branch February 5, 2023 01:23
@LecrisUT LecrisUT added this to the 2.1 milestone Feb 21, 2023
@LecrisUT LecrisUT added the enhancement Significant ehancements label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Significant ehancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants